View Issue Details

IDProjectCategoryView StatusLast Update
0038595FPCCompilerpublic2021-03-06 19:31
ReporterJ. Gareth Moreton Assigned To 
PrioritylowSeveritytweakReproducibilityN/A
Status newResolutionopen 
PlatformCross-platformOSMicrosoft Windows 
Product Version3.3.1 
Summary0038595: [Patch / Refactor] Reference helper function
DescriptionThis patch introduces a new function for the peephole optimizer named "reference_is_local". It replaces multiple conditional checks such as:

...
(taicpu(p).oper[0]^.ref^.relsymbol = nil) and
(taicpu(p).oper[0]^.ref^.segment = NR_NO) and
(taicpu(p).oper[0]^.ref^.symbol = nil) and
...

and replaces it with a single function:

reference_is_local(taicpu(p).oper[0]^.ref^)

The idea being that it reduces maintenance costs by ensuring that a check isn't missed and simplifying large conditional blocks in the peephole optimizer.
Steps To ReproduceApply patch and confirm that compiled code (e.g. the RTL) does not change.
Additional InformationThough "reference_is_local" is marked as inline, the compiler doesn't seem to like it being compiled this way and builds it as a regular function. This isn't a major concern though.

The function is designed to be cross-platform, but currently it is only utilised by the x86 peephole optimizer. It should be compatible with other platforms though.

The patch also contains an implementation for a pass-through "debug_str" function that's designed to help the compiler strip out literals in debug messages when making a release build. This isn't used anywhere yet, but if accepted, will be used in the near-future.

I had some difficulty deciding a name for the helper function; alternative names were "reference_is_regular" and "reference_is_relative". If anyone has a name that's better than "reference_is_local", feel free to change it.
Tagscompiler, patch, refactor
Fixed in Revision
FPCOldBugId
FPCTarget-
Attached Files

Activities

J. Gareth Moreton

2021-03-06 19:31

developer  

ref-helper.patch (5,421 bytes)   
Index: compiler/cgutils.pas
===================================================================
--- compiler/cgutils.pas	(revision 48887)
+++ compiler/cgutils.pas	(working copy)
@@ -199,6 +199,9 @@
     }
     function references_equal(const sref,dref : treference) : boolean;
 
+    { Returns True if the symbol fields are nil and, under x86, if the segment is NR_NO }
+    function reference_is_local(const Ref: TReference): Boolean; inline;
+
     { tlocation handling }
 
     { cannot be used for loc_(c)reference, because that one requires an alignment }
@@ -268,6 +271,19 @@
       end;
 
 
+    function reference_is_local(const Ref: TReference): Boolean; inline;
+      begin
+        Result := (Ref.relsymbol = nil) and
+{$if defined(x86)}
+                  (Ref.segment = NR_NO) and
+{$endif defined(x86)}
+{$if defined(riscv32) or defined(riscv64) or defined(arm) or defined(aarch64)}
+                  (Ref.symboldata = nil) and
+{$endif riscv32/64/arm/aarch64}
+                  (Ref.symbol = nil);
+      end;
+
+
     { returns r with the given alignment }
     function setalignment(const r : treference;b : byte) : treference;
       begin
Index: compiler/x86/aoptx86.pas
===================================================================
--- compiler/x86/aoptx86.pas	(revision 48887)
+++ compiler/x86/aoptx86.pas	(working copy)
@@ -331,9 +331,7 @@
       begin
        Result:=(ref.offset=0) and
          (ref.scalefactor in [0,1]) and
-         (ref.segment=NR_NO) and
-         (ref.symbol=nil) and
-         (ref.relsymbol=nil) and
+         reference_is_local(ref) and
          ((base=NR_INVALID) or
           (ref.base=base)) and
          ((index=NR_INVALID) or
@@ -345,9 +343,7 @@
     function MatchReferenceWithOffset(const ref : treference;base,index : TRegister) : Boolean;
       begin
        Result:=(ref.scalefactor in [0,1]) and
-         (ref.segment=NR_NO) and
-         (ref.symbol=nil) and
-         (ref.relsymbol=nil) and
+         reference_is_local(ref) and
          ((base=NR_INVALID) or
           (ref.base=base)) and
          ((index=NR_INVALID) or
@@ -861,6 +857,12 @@
         asml.insertbefore(tai_comment.Create(strpnew(s)), p);
       end;
 
+    { Pass through string - used to help optimise release builds by stripping out debug literals }
+    function debug_str(const s: string): string; inline;
+      begin
+        Result := s;
+      end;
+
     function debug_tostr(i: tcgint): string; inline;
       begin
         Result := tostr(i);
@@ -921,6 +923,11 @@
       begin
       end;
 
+    function debug_str(const s: string); string; inline;
+      begin
+        Result := '';
+      end;
+
     function debug_tostr(i: tcgint): string; inline;
       begin
         Result := '';
@@ -3412,12 +3419,8 @@
             { Check common LEA/LEA conditions }
             if MatchInstruction(hp1,A_LEA,[taicpu(p).opsize]) and
               (taicpu(p).oper[1]^.reg = taicpu(hp1).oper[1]^.reg) and
-              (taicpu(p).oper[0]^.ref^.relsymbol = nil) and
-              (taicpu(p).oper[0]^.ref^.segment = NR_NO) and
-              (taicpu(p).oper[0]^.ref^.symbol = nil) and
-              (taicpu(hp1).oper[0]^.ref^.relsymbol = nil) and
-              (taicpu(hp1).oper[0]^.ref^.segment = NR_NO) and
-              (taicpu(hp1).oper[0]^.ref^.symbol = nil) and
+              reference_is_local(taicpu(p).oper[0]^.ref^) and
+              reference_is_local(taicpu(hp1).oper[0]^.ref^) and
               (
                 (taicpu(p).oper[0]^.ref^.base = NR_NO) or { Don't call RegModifiedBetween unnecessarily }
                 not(RegModifiedBetween(taicpu(p).oper[0]^.ref^.base,p,hp1))
@@ -3806,9 +3809,7 @@
                 if taicpu(hp1).opcode=A_LEA then
                   begin
                     if (TmpRef.base = NR_NO) and
-                       (taicpu(hp1).oper[0]^.ref^.symbol=nil) and
-                       (taicpu(hp1).oper[0]^.ref^.relsymbol=nil) and
-                       (taicpu(hp1).oper[0]^.ref^.segment=NR_NO) and
+                       reference_is_local(taicpu(hp1).oper[0]^.ref^) and
                        ((taicpu(hp1).oper[0]^.ref^.scalefactor=0) or
                        (taicpu(hp1).oper[0]^.ref^.scalefactor*tmpref.scalefactor<=8)) then
                       begin
@@ -7224,9 +7225,7 @@
             higher values are unlikely }
           ((taicpu(p).oper[0]^.ref^.offset=-8) or
            (taicpu(p).oper[0]^.ref^.offset=-24))  and
-          (taicpu(p).oper[0]^.ref^.symbol=nil) and
-          (taicpu(p).oper[0]^.ref^.relsymbol=nil) and
-          (taicpu(p).oper[0]^.ref^.segment=NR_NO) and
+          reference_is_local(taicpu(p).oper[0]^.ref^) and
           (taicpu(p).oper[1]^.reg=NR_STACK_POINTER_REG) and
           GetNextInstruction(p, hp1) and
           { Take a copy of hp1 }
@@ -7241,9 +7240,7 @@
           (taicpu(hp2).oper[0]^.ref^.offset=-taicpu(p).oper[0]^.ref^.offset) and
           (taicpu(hp2).oper[0]^.ref^.base=NR_STACK_POINTER_REG) and
           (taicpu(hp2).oper[0]^.ref^.index=NR_NO) and
-          (taicpu(hp2).oper[0]^.ref^.symbol=nil) and
-          (taicpu(hp2).oper[0]^.ref^.relsymbol=nil) and
-          (taicpu(hp2).oper[0]^.ref^.segment=NR_NO) and
+          reference_is_local(taicpu(hp2).oper[0]^.ref^) and
           (taicpu(hp2).oper[1]^.reg=NR_STACK_POINTER_REG) and
           GetNextInstruction(hp2, hp3) and
           { trick to skip label }
ref-helper.patch (5,421 bytes)   

Issue History

Date Modified Username Field Change
2021-03-06 19:31 J. Gareth Moreton New Issue
2021-03-06 19:31 J. Gareth Moreton File Added: ref-helper.patch
2021-03-06 19:31 J. Gareth Moreton Tag Attached: patch
2021-03-06 19:31 J. Gareth Moreton Tag Attached: compiler
2021-03-06 19:31 J. Gareth Moreton Tag Attached: refactor
2021-03-06 19:31 J. Gareth Moreton Priority normal => low
2021-03-06 19:31 J. Gareth Moreton Severity minor => tweak
2021-03-06 19:31 J. Gareth Moreton FPCTarget => -