Re: [fpc-devel] Review of AVR patch for bug 31925
Am 22.09.2017 um 17:28 schrieb Christo: > On Fri, 2017-09-22 at 07:10 +0200, Christo wrote: >> On Wed, 2017-09-20 at 12:36 +0200, Karoly Balogh (Charlie/SGR) wrote: >>> > > A complication I've noted is that enabling overflow checking doesn't > call the fpc_mul_byte_overflow function (as the naming convention > suggested), rather the generated code calls fpc_mul_byte and then check > the carry flag to determine whether an overflow occurred. Since the > function in generic.inc is plain pascal code there is no guarantee that > the carry flag will be set if an overflow should have occurred. I > assume that at a higher level the code generator assumes that a > hardware MUL instruction _will_ be used, leading to incorrect overflow > detection logic if a software helper is used. > > A similar situation exist for the current 16 bit software MUL case with > overflow checking. > > Any pointers on where to start looking to try and adapt the overflow > checking code generation? AVR has no working overflow checking yet. Basically, tcg.a_op_reg_reg_reg_checkoverflow and tcg.a_op_conbst_reg_reg_checkoverflow need to be overridden and they have to set ovloc properly which is checked later on by g_overflowCheck_loc, ___ fpc-devel maillist - fpc-devel@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
Re: [fpc-devel] Review of AVR patch for bug 31925
implementing 8*8 bit multiplications as library functions is certainly one way to cope with the absence of mul/mulsu instructions in the "basic" avr subarchitectures, and the resultint compiler error message when confronted with any source code resulting in such a multiplication. The question is, if that is the most reasonable way to do it, or if it would be simpler to have the compiler directly insert the required compare/add/shift sequence. This should be answered by someone with a very good knowledge of the AVR code generator. Am 22.09.2017 um 12:28 schrieb Christo: On Fri, 2017-09-22 at 07:10 +0200, Christo wrote: On Wed, 2017-09-20 at 12:36 +0200, Karoly Balogh (Charlie/SGR) wrote: A complication I've noted is that enabling overflow checking doesn't call the fpc_mul_byte_overflow function (as the naming convention suggested), rather the generated code calls fpc_mul_byte and then check the carry flag to determine whether an overflow occurred. Since the function in generic.inc is plain pascal code there is no guarantee that the carry flag will be set if an overflow should have occurred. I assume that at a higher level the code generator assumes that a hardware MUL instruction _will_ be used, leading to incorrect overflow detection logic if a software helper is used. A similar situation exist for the current 16 bit software MUL case with overflow checking. Any pointers on where to start looking to try and adapt the overflow checking code generation? ___ fpc-devel maillist - fpc-devel@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel ___ fpc-devel maillist - fpc-devel@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
Re: [fpc-devel] Review of AVR patch for bug 31925
On Wed, 2017-09-20 at 12:36 +0200, Karoly Balogh (Charlie/SGR) wrote: > Few things I see: I wouldn't do IFDEF CPUAVR in inc/generic and > inc/compproc. Use maybe IFDEF CPU8 and/or CPU16 instead. This code is > indeed generic, and will be useful on other limited CPU archs as > well. Or > just follow the structure of other MUL helpers, and don't ifdef based > on > the CPU width at all. The linker will (should) optimize away this > code on > architectures where it is unnecessary. > > Also, instead of copypasting the helper function call code, I'd think > about generalizing it, and extracting it to a private method of > tcgavr, > but as I'm not the maintainer of the AVR cg, i'm not sure this is > what > they would want there. :) Thanks for the suggestions Karoly. I will remove the ifdefs, this makes the code a bit cleaner. I don't understand the details of tcg(avr) so it would be challenging for me to try refactoring code. ___ fpc-devel maillist - fpc-devel@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
Re: [fpc-devel] Review of AVR patch for bug 31925
Hi, On Wed, 20 Sep 2017, Christo wrote: > I have made an attempt at fixing an AVR related bug. Since this is my > first attempt at messing with the compiler, I would like a review and > critique of my attempt. > (...) > > Can anyone see any problems with this approach, or perhaps suggest a > better approach to fix this bug? I don't know a lot about AVR, and haven't actually tried your patch. But I took a look at it. Few things I see: I wouldn't do IFDEF CPUAVR in inc/generic and inc/compproc. Use maybe IFDEF CPU8 and/or CPU16 instead. This code is indeed generic, and will be useful on other limited CPU archs as well. Or just follow the structure of other MUL helpers, and don't ifdef based on the CPU width at all. The linker will (should) optimize away this code on architectures where it is unnecessary. Also, instead of copypasting the helper function call code, I'd think about generalizing it, and extracting it to a private method of tcgavr, but as I'm not the maintainer of the AVR cg, i'm not sure this is what they would want there. :) But in general I'd say it's the right approach, at least we used this elsewhere too. Charlie___ fpc-devel maillist - fpc-devel@lists.freepascal.org http://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
[fpc-devel] Review of AVR patch for bug 31925
I have made an attempt at fixing an AVR related bug. Since this is my
first attempt at messing with the compiler, I would like a review and
critique of my attempt.
The bug seems to be caused by the absence of the MUL instruction for
the limited AVR subarchitectures (avr1 - avr35). As far as I could see
there is no fallback code for the case where CPUAVR_HAS_MUL is not
defined for an 8 bit MUL operation. I have copied the code for the
corresponding 16 bit MUL branch, then copied and adapted the pascal
code for fpc_mul_xxx etc in rtl/inc/generic.inc and added declarations
to rtl/inc/generic.inc.
Can anyone see any problems with this approach, or perhaps suggest a
better approach to fix this bug?
ThanksIndex: compiler/avr/cgcpu.pas
===
--- compiler/avr/cgcpu.pas (revision 36776)
+++ compiler/avr/cgcpu.pas (working copy)
@@ -609,7 +609,36 @@
cg.a_reg_dealloc(list,NR_R0);
end
else
- internalerror(2015061001);
+ begin
+ { keep code for muls with overflow checking }
+ if size=OS_8 then
+ pd:=search_system_proc('fpc_mul_byte')
+ else
+ pd:=search_system_proc('fpc_mul_shortint');
+ paraloc1.init;
+ paraloc2.init;
+ paramanager.getintparaloc(list,pd,1,paraloc1);
+ paramanager.getintparaloc(list,pd,2,paraloc2);
+ a_load_reg_cgpara(list,OS_8,src,paraloc2);
+ a_load_reg_cgpara(list,OS_8,dst,paraloc1);
+ paramanager.freecgpara(list,paraloc2);
+ paramanager.freecgpara(list,paraloc1);
+ alloccpuregisters(list,R_INTREGISTER,paramanager.get_volatile_registers_int(pocall_default));
+ if size=OS_8 then
+ a_call_name(list,'FPC_MUL_BYTE',false)
+ else
+ a_call_name(list,'FPC_MUL_SHORTINT',false);
+ dealloccpuregisters(list,R_INTREGISTER,paramanager.get_volatile_registers_int(pocall_default));
+ cg.a_reg_alloc(list,NR_R24);
+ cg.a_reg_alloc(list,NR_R25);
+ cg.a_load_reg_reg(list,OS_8,OS_8,NR_R24,dst);
+ cg.a_reg_dealloc(list,NR_R24);
+ cg.a_load_reg_reg(list,OS_8,OS_8,NR_R25,GetNextReg(dst));
+ cg.a_reg_dealloc(list,NR_R25);
+ paraloc2.done;
+ paraloc1.done;
+ end;
+ //internalerror(2015061001);
end
else if size in [OS_16,OS_S16] then
begin
Index: rtl/inc/compproc.inc
===
--- rtl/inc/compproc.inc (revision 36776)
+++ rtl/inc/compproc.inc (working copy)
@@ -598,6 +598,14 @@
function fpc_mul_longint(f1,f2 : longint;checkoverflow : boolean) : longint; compilerproc;
function fpc_mul_dword(f1,f2 : dword;checkoverflow : boolean) : dword; compilerproc;
{$else VER3_0}
+
+{$if defined(CPUAVR) and not defined(CPUAVR_HAS_MUL)}
+function fpc_mul_shortint(f1,f2 : shortint) : shortint; compilerproc;
+function fpc_mul_shortint_checkoverflow(f1,f2 : shortint) : shortint; compilerproc;
+function fpc_mul_byte(f1,f2 : byte) : byte; compilerproc;
+function fpc_mul_byte_checkoverflow(f1,f2 : byte) : byte; compilerproc;
+{$endif CPUAVR_HAS_MUL}
+
function fpc_mul_integer(f1,f2 : integer) : integer; compilerproc;
function fpc_mul_integer_checkoverflow(f1,f2 : integer) : integer; compilerproc;
function fpc_mul_word(f1,f2 : word) : word; compilerproc;
Index: rtl/inc/generic.inc
===
--- rtl/inc/generic.inc (revision 36776)
+++ rtl/inc/generic.inc (working copy)
@@ -1500,6 +1500,109 @@
{$else VER3_0}
+{$if defined(CPUAVR) AND not defined(CPUAVR_HAS_MUL)}
+{$ifndef FPC_SYSTEM_HAS_MUL_SHORTINT}
+function fpc_mul_shortint(f1,f2 : shortint) : shortint;[public,alias: 'FPC_MUL_SHORTINT']; compilerproc;
+ begin
+{ there's no difference between signed and unsigned multiplication,
+ when the destination size is equal to the source size and overflow
+ checking is off }
+{ byte(f1) * byte(f2) is coded as a call to mulword }
+fpc_mul_shortint := shortint(byte(f1) * byte(f2));
+ end;
+
+function fpc_mul_shortint_checkoverflow(f1,f2 : shortint) : shortint;[public,alias: 'FPC_MUL_SHORTINT_CHECKOVERFLOW']; compilerproc;
+ var
+sign : boolean;
+q1,q2,q3 : byte;
+ begin
+sign:=false;
+if f1 < 0 then
+ begin
+sign := not(sign);
+q1 := byte(-f1);
+
