View Issue Details

IDProjectCategoryView StatusLast Update
0036079FPCCompilerpublic2019-09-20 20:55
ReporterChristo CrauseAssigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Product Version3.3.1Product Build 
Target VersionFixed in Version 
Summary0036079: AVR - Const value passed as parameter to procedure clobbered by peephole optimizer
DescriptionThe test program (below in steps to reproduce) succeeds when compiled without optimization, but fails when compiled with optimization (-O1). The problem seems to be related to passing a constant as second parameter to procedure tests. When compiled without optimization registers r10..r17 are initialized correctly before calling procedure test (snippet 0000001 below). When compiled with -O1 registers r11 and r12 are eliminated by peephole optimization MovOp2Op.

It appears that the registers containing the constant value gets deallocated before the function call, hence the optimizer see the mov instructions as available for elimination.
Steps To Reproduceprogram tshlshr;

procedure test(value, required: int64);
begin
  if value <> required then
    halt(1)
  else
    halt(0);
end;

var
 longres : longint;

begin
   longres := 32768;
   test(longres, 32768);
end.
Additional InformationSnippet 0000001:
 158: e1 2c mov r14, r1
 15a: f1 2c mov r15, r1
 15c: 01 2d mov r16, r1
 15e: 11 2d mov r17, r1
 160: a1 2c mov r10, r1
 162: a0 e8 ldi r26, 0x80 ; 128
 164: ba 2e mov r11, r26
 166: c1 2c mov r12, r1
 168: d1 2c mov r13, r1
 16a: 0e 94 1d 00 call 0x3a ; 0x3a <PsTSHLSHR_ss_TESTsINT64sINT64>

Snippet 0000002:
 14e: e1 2c mov r14, r1
 150: f1 2c mov r15, r1
 152: 01 2d mov r16, r1
 154: 11 2d mov r17, r1
 156: a1 2c mov r10, r1
 158: a0 e8 ldi r26, 0x80 ; 128
 15a: d1 2c mov r13, r1
 15c: 0e 94 1d 00 call 0x3a ; 0x3a <PsTSHLSHR_ss_TESTsINT64sINT64>
TagsAVR
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files
  • ncgcal.patch (1,468 bytes)
    diff --git compiler/ncgcal.pas compiler/ncgcal.pas
    index c861f59e36..60fdbb3040 100644
    --- compiler/ncgcal.pas
    +++ compiler/ncgcal.pas
    @@ -1054,8 +1054,6 @@ implementation
                        begin
                          reorder_parameters;
                          pushparas;
    -                     { free the resources allocated for the parameters }
    -                     freeparas;
                        end;
     
                      if callref then
    @@ -1091,8 +1089,6 @@ implementation
                         begin
                           reorder_parameters;
                           pushparas;
    -                      { free the resources allocated for the parameters }
    -                      freeparas;
                         end;
     
                       cg.alloccpuregisters(current_asmdata.CurrAsmList,R_INTREGISTER,regs_to_save_int);
    @@ -1152,8 +1148,6 @@ implementation
                     begin
                       reorder_parameters;
                       pushparas;
    -                  { free the resources allocated for the parameters }
    -                  freeparas;
                     end;
     
                   if callref then
    @@ -1181,6 +1175,10 @@ implementation
                   extra_post_call_code;
                end;
     
    +         { free the resources allocated for the parameters }
    +         if assigned(left) then
    +           freeparas;
    +
              { Need to remove the parameters from the stack? }
              if (procdefinition.proccalloption in clearstack_pocalls) then
               begin
    
    ncgcal.patch (1,468 bytes)

Activities

Christo Crause

2019-09-17 22:20

reporter   ~0118100

It seems as if this problem occurs when procedure parameters require registers below r18 which are only initialized by MOV, directly followed by a register deallocation.

I suspect this situation gets created in tcgcallnode.pass_generate_code because the call to freeparas which deallocates all parameter registers, followed by cg.alloccpuregisters(...,regs_to_save_int) which only allocates the volatile registers, followed by the procedure call.

At first it seems as if the freeparas method should only be called after the procedure call, this way the optimizer will be prevented from removing the MOV instruction(s) because they are still allocated. This is however a tricky place in the compiler and I cannot see all the consequences of swapping processing at this level.

It is difficult to see how the optimizer checks can be improved to filter out this situation.

Christo Crause

2019-09-17 22:22

reporter   ~0118101

A further note - this particular situation is only triggered when compiling with -O1 or -O2. The code seems good when compiled with -O3/4.

Christo Crause

2019-09-19 21:13

reporter   ~0118121

Attached please find a patch which moves the freeparas method to below the a_call_XX methods which generate the actual call. This ensures that the parameter registers which are not part of the volatile registers set remain allocated until after the procedure/function call. The compiler test suite was ran with this patch for x86_64-linux and compared with the unpatched compiler - no changes in the results were observed.

Of course this patch also fixes the original problem noted for avr-embedded.

ncgcal.patch (1,468 bytes)
diff --git compiler/ncgcal.pas compiler/ncgcal.pas
index c861f59e36..60fdbb3040 100644
--- compiler/ncgcal.pas
+++ compiler/ncgcal.pas
@@ -1054,8 +1054,6 @@ implementation
                    begin
                      reorder_parameters;
                      pushparas;
-                     { free the resources allocated for the parameters }
-                     freeparas;
                    end;
 
                  if callref then
@@ -1091,8 +1089,6 @@ implementation
                     begin
                       reorder_parameters;
                       pushparas;
-                      { free the resources allocated for the parameters }
-                      freeparas;
                     end;
 
                   cg.alloccpuregisters(current_asmdata.CurrAsmList,R_INTREGISTER,regs_to_save_int);
@@ -1152,8 +1148,6 @@ implementation
                 begin
                   reorder_parameters;
                   pushparas;
-                  { free the resources allocated for the parameters }
-                  freeparas;
                 end;
 
               if callref then
@@ -1181,6 +1175,10 @@ implementation
               extra_post_call_code;
            end;
 
+         { free the resources allocated for the parameters }
+         if assigned(left) then
+           freeparas;
+
          { Need to remove the parameters from the stack? }
          if (procdefinition.proccalloption in clearstack_pocalls) then
           begin
ncgcal.patch (1,468 bytes)

Issue History

Date Modified Username Field Change
2019-09-15 22:01 Christo Crause New Issue
2019-09-17 11:32 Dimitrios Chr. Ioannidis Tag Attached: AVR
2019-09-17 22:20 Christo Crause Note Added: 0118100
2019-09-17 22:22 Christo Crause Note Added: 0118101
2019-09-19 21:13 Christo Crause File Added: ncgcal.patch
2019-09-19 21:13 Christo Crause Note Added: 0118121