View Issue Details

IDProjectCategoryView StatusLast Update
0036553FPCCompilerpublic2020-01-12 11:33
ReporterJ. Gareth MoretonAssigned ToFlorian 
PrioritylowSeveritytweakReproducibilityN/A
Status resolvedResolutionfixed 
Platformi386 and x86_64OSMicrosoft WindowsOS Version10 Professional
Product Version3.3.1Product Buildr43909 
Target VersionFixed in Version3.3.1 
Summary0036553: [Refactor] Some cleaning up of OptPass2JMP and OptPass2MOV
DescriptionThese two patches, while not fixing any known bugs, aim to fix a couple of issues that might affect the maintainability and extensibility of the x86 Peephole Optimizer in the future. See Additional Information for more details.
Steps To ReproduceApply patches, verify that code has less 'smell' and is potentially safer, and confirm that the binaries of independent code built by the compiler haven't changed.
Additional InformationOptPass2JMP - this patch adds calls to 'AllocRegBetween' for the 'JMP -> MOV/RET' optimisation, so the registers referenced by the duplicated MOV instruction are properly tracked.

OptPass2MOV - honestly, I don't know what I was thinking here other than 'micro-optimisation'! I stripped out some code that does some trickery based on some assumptions about the upcoming instructions (which are generally rather sound) and the flow of the Peephole Optimizer... all to avoid calling OptPass2JMP on a jump that previous knowledge has determined will return False. The main thing is that if a future change is made so Pass 2 is called multiple times, or it jumps back to Pass 1, the trick would have potentially caused an infinite loop.
Tagscompiler, i386, optimizations, patch, refactor, x86_64
Fixed in Revision43919
FPCOldBugId
FPCTarget-
Attached Files
  • OptPass2MOV-refactor.patch (1,881 bytes)
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 43909)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -3384,30 +3384,10 @@
                   { OptPass2MOV will now exit but will be called again if OptPass1MOV
                     returned True and the instruction is still a MOV, thus checking
                     the optimisations below }
    -            else
    -              { Since OptPass2JMP returned false, no optimisations were done to
    -                the jump. Additionally, a label will definitely follow the jump
    -                (although it may have become dead), so skip ahead as far as
    -                possible }
    -              begin
    -                while (p <> hp1) do
    -                  begin
    -                    { Nothing changed between the MOV and the JMP, so
    -                      don't bother with "UpdateUsedRegsAndOptimize" }
    -                    UpdateUsedRegs(p);
    -                    p := tai(p.Next);
    -                  end;
     
    -                { Use "UpdateUsedRegsAndOptimize" here though, because the
    -                  label might now be dead and can be stripped out }
    -                p := tai(UpdateUsedRegsAndOptimize(hp1).Next);
    -
    -                { If p is a label, then Result will be False and program flow
    -                  will move onto the next list entry in "PeepHoleOptPass2" }
    -                if (p = BlockEnd) or not (p.typ in [ait_align, ait_label]) then
    -                  Result := True;
    -
    -              end;
    +            { If OptPass2JMP returned False, no optimisations were done to
    +              the jump and there are no further optimisations that can be done
    +              to the MOV instruction on this pass }
               end
             else if MatchOpType(taicpu(p),top_reg,top_reg) and
     {$ifdef x86_64}
    
  • OptPass2JMP-refactor.patch (2,297 bytes)
    Index: compiler/x86/aoptx86.pas
    ===================================================================
    --- compiler/x86/aoptx86.pas	(revision 43909)
    +++ compiler/x86/aoptx86.pas	(working copy)
    @@ -3740,7 +3740,8 @@
     
         function TX86AsmOptimizer.OptPass2Jmp(var p : tai) : boolean;
           var
    -        hp1, hp2 : tai;
    +        hp1, hp2, hp3: tai;
    +        OperIdx: Integer;
           begin
             result:=false;
             if (taicpu(p).oper[0]^.typ=top_ref) and (taicpu(p).oper[0]^.ref^.refaddr=addr_full) and (taicpu(p).oper[0]^.ref^.base=NR_NO) and
    @@ -3790,8 +3791,28 @@
                             if Assigned(hp2) and MatchInstruction(hp2, A_RET, [S_NO]) then
                               begin
                                 { Duplicate the MOV instruction }
    -                            asml.InsertBefore(hp1.getcopy, p);
    +                            hp3 := tai(hp1.getcopy);
    +                            asml.InsertBefore(hp3, p);
     
    +                            { Make sure the compiler knows about any final registers written here }
    +                            for OperIdx := 0 to 1 do
    +                              with taicpu(hp3).oper[OperIdx]^ do
    +                                begin
    +                                  case typ of
    +                                    top_ref:
    +                                      begin
    +                                        if (ref^.base <> NR_NO) and (ref^.base <> NR_RIP) then
    +                                          AllocRegBetween(ref^.base, hp3, tai(p.Next), UsedRegs);
    +                                        if (ref^.index <> NR_NO) and (ref^.index <> NR_RIP) then
    +                                          AllocRegBetween(ref^.index, hp3, tai(p.Next), UsedRegs);
    +                                      end;
    +                                    top_reg:
    +                                      AllocRegBetween(reg, hp3, tai(p.Next), UsedRegs);
    +                                    else
    +                                      { Do nothing };
    +                                  end;
    +                                end;
    +
                                 { Now change the jump into a RET instruction }
                                 ConvertJumpToRET(p, hp2);
                                 result:=true;
    z

Activities

J. Gareth Moreton

2020-01-11 04:58

developer  

OptPass2MOV-refactor.patch (1,881 bytes)
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 43909)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -3384,30 +3384,10 @@
               { OptPass2MOV will now exit but will be called again if OptPass1MOV
                 returned True and the instruction is still a MOV, thus checking
                 the optimisations below }
-            else
-              { Since OptPass2JMP returned false, no optimisations were done to
-                the jump. Additionally, a label will definitely follow the jump
-                (although it may have become dead), so skip ahead as far as
-                possible }
-              begin
-                while (p <> hp1) do
-                  begin
-                    { Nothing changed between the MOV and the JMP, so
-                      don't bother with "UpdateUsedRegsAndOptimize" }
-                    UpdateUsedRegs(p);
-                    p := tai(p.Next);
-                  end;
 
-                { Use "UpdateUsedRegsAndOptimize" here though, because the
-                  label might now be dead and can be stripped out }
-                p := tai(UpdateUsedRegsAndOptimize(hp1).Next);
-
-                { If p is a label, then Result will be False and program flow
-                  will move onto the next list entry in "PeepHoleOptPass2" }
-                if (p = BlockEnd) or not (p.typ in [ait_align, ait_label]) then
-                  Result := True;
-
-              end;
+            { If OptPass2JMP returned False, no optimisations were done to
+              the jump and there are no further optimisations that can be done
+              to the MOV instruction on this pass }
           end
         else if MatchOpType(taicpu(p),top_reg,top_reg) and
 {$ifdef x86_64}

J. Gareth Moreton

2020-01-11 07:00

developer  

OptPass2JMP-refactor.patch (2,297 bytes)
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 43909)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -3740,7 +3740,8 @@
 
     function TX86AsmOptimizer.OptPass2Jmp(var p : tai) : boolean;
       var
-        hp1, hp2 : tai;
+        hp1, hp2, hp3: tai;
+        OperIdx: Integer;
       begin
         result:=false;
         if (taicpu(p).oper[0]^.typ=top_ref) and (taicpu(p).oper[0]^.ref^.refaddr=addr_full) and (taicpu(p).oper[0]^.ref^.base=NR_NO) and
@@ -3790,8 +3791,28 @@
                         if Assigned(hp2) and MatchInstruction(hp2, A_RET, [S_NO]) then
                           begin
                             { Duplicate the MOV instruction }
-                            asml.InsertBefore(hp1.getcopy, p);
+                            hp3 := tai(hp1.getcopy);
+                            asml.InsertBefore(hp3, p);
 
+                            { Make sure the compiler knows about any final registers written here }
+                            for OperIdx := 0 to 1 do
+                              with taicpu(hp3).oper[OperIdx]^ do
+                                begin
+                                  case typ of
+                                    top_ref:
+                                      begin
+                                        if (ref^.base <> NR_NO) and (ref^.base <> NR_RIP) then
+                                          AllocRegBetween(ref^.base, hp3, tai(p.Next), UsedRegs);
+                                        if (ref^.index <> NR_NO) and (ref^.index <> NR_RIP) then
+                                          AllocRegBetween(ref^.index, hp3, tai(p.Next), UsedRegs);
+                                      end;
+                                    top_reg:
+                                      AllocRegBetween(reg, hp3, tai(p.Next), UsedRegs);
+                                    else
+                                      { Do nothing };
+                                  end;
+                                end;
+
                             { Now change the jump into a RET instruction }
                             ConvertJumpToRET(p, hp2);
                             result:=true;
z

Florian

2020-01-12 11:33

administrator   ~0120355

Thanks, applied (slightly modified).

Issue History

Date Modified Username Field Change
2020-01-11 04:58 J. Gareth Moreton New Issue
2020-01-11 04:58 J. Gareth Moreton File Added: OptPass2JMP-refactor.patch
2020-01-11 04:58 J. Gareth Moreton File Added: OptPass2MOV-refactor.patch
2020-01-11 04:58 J. Gareth Moreton Tag Attached: patch
2020-01-11 04:58 J. Gareth Moreton Tag Attached: compiler
2020-01-11 04:58 J. Gareth Moreton Tag Attached: optimizations
2020-01-11 04:58 J. Gareth Moreton Tag Attached: refactor
2020-01-11 04:58 J. Gareth Moreton Tag Attached: i386
2020-01-11 04:58 J. Gareth Moreton Tag Attached: x86_64
2020-01-11 04:59 J. Gareth Moreton Priority normal => low
2020-01-11 04:59 J. Gareth Moreton Severity minor => tweak
2020-01-11 04:59 J. Gareth Moreton FPCTarget => -
2020-01-11 05:00 J. Gareth Moreton Additional Information Updated View Revisions
2020-01-11 06:11 J. Gareth Moreton File Deleted: OptPass2JMP-refactor.patch
2020-01-11 06:12 J. Gareth Moreton File Added: OptPass2JMP-refactor.patch
2020-01-11 06:59 J. Gareth Moreton File Deleted: OptPass2JMP-refactor.patch
2020-01-11 06:59 J. Gareth Moreton File Added: OptPass2JMP-refactor.patch
2020-01-11 07:00 J. Gareth Moreton File Deleted: OptPass2JMP-refactor.patch
2020-01-11 07:00 J. Gareth Moreton File Added: OptPass2JMP-refactor.patch
2020-01-12 11:33 Florian Assigned To => Florian
2020-01-12 11:33 Florian Status new => resolved
2020-01-12 11:33 Florian Resolution open => fixed
2020-01-12 11:33 Florian Fixed in Version => 3.3.1
2020-01-12 11:33 Florian Fixed in Revision => 43919
2020-01-12 11:33 Florian Note Added: 0120355