View Issue Details

IDProjectCategoryView StatusLast Update
0038194FPCCompilerpublic2021-03-25 22:54
ReporterJ. Gareth Moreton Assigned ToFlorian  
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
PlatformCross-platformOSMicrosoft Windows 
Product Version3.3.1 
Fixed in Version3.3.1 
Summary0038194: [Patch / Refactor] Nothing (NOP) node optimisation
DescriptionWith thanks to 0038156, I was able to develop this patch that aims to improve TBlockNode.simplify by removing statements that contain "nothing" nodes, and merging blocks together when possible.

Besides removing the node tree of clutter (as confirmed when the compiler is built with DEBUG_NODE_XML), it aims to speed up compilation slightly by removing the number of nodes that have to be processed by the second pass as well as potentially allowing for additional node-level optimisations in the future.

Generated binaries should not change - differences in the final binaries are indictive of a bug or oversight.
Steps To ReproduceApply patch and comfirm identical generated binaries but cleaner node trees and possible speed boost.
Additional InformationWhen assembler files are preserved, labels may get renamed if block nodes get merged. Though this shows up as a difference when compared side-by-side with the trunk, the output binary is still identical.
Tagscompiler, node, patch, refactor
Fixed in Revision49054
FPCOldBugId
FPCTarget-
Attached Files

Activities

J. Gareth Moreton

2020-12-15 17:34

developer   ~0127623

Not to be pushy, but this optimisation, if it doesn't break something, will aid in my pure function research and implementation because it simplifies the node tree that might otherwise be cluttered with nothing nodes but ultimately collapse into something simple (also allows faster processing when constants are propagated through it).

J. Gareth Moreton

2021-02-02 06:34

developer   ~0128733

Brought the patch up to date and fixed a possible problem should "break_inlining" be defined.

Florian

2021-02-02 22:15

administrator   ~0128747

On linux, the patch causes a lot of new failures:

test/cg/tcalcst6
test/cg/tcalvar6
test/tcasecov1
test/tcasecov2
test/tcasecov3
test/tcasecov3a
test/tcasecov3b
test/tconstref4
test/textpas01
test/tisobuf1
test/tisoext1
test/tisoext4
test/tisoread
test/tisorec1
test/tisorec4
test/tisorec5
test/tsafecall1
test/tsafecall2
test/tsafecall3
test/tsafecall4
tbs/tb0564
webtbs/tw12237
webtbs/tw16668
webtbs/tw16757
webtbs/tw22561
webtbs/tw24318
webtbs/tw26271
webtbs/tw27522
webtbs/tw28475
webtbs/tw28948
webtbs/tw32938
webtbs/tw34848
webtbs/tw35136
webtbs/tw35626
webtbs/tw37085
webtbs/tw37154
webtbs/tw37428
webtbs/tw37823
webtbs/tw8183
webtbs/tw8523
webtbs/tw8977

J. Gareth Moreton

2021-02-03 10:52

developer   ~0128754

Curious. I'll do some investigating. This is x86_64-linux presumably.

Florian

2021-02-03 20:08

administrator   ~0128759

Yes, x86_64-linux.

J. Gareth Moreton

2021-02-11 16:31

developer   ~0128895

Last edited: 2021-02-11 16:33

View 3 revisions

What were your build and test parameters, Florian? I just tested it now on x86_64-linux (Ubuntu on a virtual machine) with the default "make clean all" for building the compiler, and "make full" for the tests, and the output logs were identical for the trunk and the trunk with the patch applied.

J. Gareth Moreton

2021-03-07 11:24

developer   ~0129466

Okay, finally managed to reproduce the errors, or at least some of them:

Failed to compile test/cg/tcalcst6.pp 2020/09/06 18:13:36 internalerror generated

Time to find out what's going on!

J. Gareth Moreton

2021-03-09 03:11

developer   ~0129522

Fixed the bug - it was due to some nodes getting stripped that had the nf_usercode_entry flag set.

Hard to say if it's worth it ultimately. Node tree is potentially simplified slightly but observed changes in assembly output seems restricted to more efficient usage of label numbers.
nothing-node-simplify.patch (6,268 bytes)   
Index: compiler/nbas.pas
===================================================================
--- compiler/nbas.pas	(revision 48908)
+++ compiler/nbas.pas	(working copy)
@@ -706,10 +706,31 @@
 
 
     function tblocknode.simplify(forinline : boolean): tnode;
+      var
+        hp, nextp: TStatementNode;
+        lastp: TNode;
 {$ifdef break_inlining}
-      var
         a : array[0..3] of tstatementnode;
 {$endif break_inlining}
+
+        procedure EraseCurrentStatement;
+          begin
+            { make sure the nf_block_with_exit and nf_usercode_entry flags are safeguarded }
+            if Assigned(nextp) then
+              nextp.flags := nextp.flags + (hp.left.flags * [nf_block_with_exit, nf_usercode_entry]);
+
+            hp.right := nil;
+            hp.Free;
+            hp := nextp;
+            if not Assigned(lastp) then
+              Exit;
+
+            if lastp = Self then
+              TBlockNode(lastp).left := nextp
+            else
+              TStatementNode(lastp).right := nextp;
+          end;
+
       begin
         result := nil;
         { Warning: never replace a blocknode with another node type,
@@ -717,33 +738,93 @@
           main program body, and those nodes should always be blocknodes
           since that's what the compiler expects elsewhere. }
 
-        if assigned(left) and
-           not assigned(tstatementnode(left).right) then
+        lastp := Self;
+        hp := TStatementNode(left);
+
+        if Assigned(hp) then
           begin
-            case tstatementnode(left).left.nodetype of
-              blockn:
-                begin
-                  { if the current block contains only one statement, and
-                    this one statement only contains another block, replace
-                    this block with that other block.                       }
-                  result:=tstatementnode(left).left;
-                  tstatementnode(left).left:=nil;
-                  { make sure the nf_block_with_exit flag is safeguarded }
-                  result.flags:=result.flags+(flags*[nf_block_with_exit,nf_usercode_entry]);
-                  exit;
+            if not assigned(tstatementnode(left).right) then
+              begin
+                { Simplify single-statement blocks }
+                case tstatementnode(left).left.nodetype of
+                  blockn:
+                    begin
+                      { if the current block contains only one statement, and
+                        this one statement only contains another block, replace
+                        this block with that other block.                       }
+                      result:=tstatementnode(left).left;
+                      tstatementnode(left).left:=nil;
+                      { make sure the nf_block_with_exit and nf_usercode_entry flags are safeguarded }
+                      result.flags:=result.flags+(flags*[nf_block_with_exit,nf_usercode_entry]);
+                      exit;
+                    end;
+                  nothingn:
+                    begin
+                      { if the block contains only a statement with a nothing node,
+                        get rid of the statement }
+                      left.Free;
+                      left:=nil;
+                      exit;
+                    end;
+                  else
+                    ;
                 end;
-              nothingn:
-                begin
-                  { if the block contains only a statement with a nothing node,
-                    get rid of the statement }
-                  left.Free;
-                  left:=nil;
-                  exit;
+              end
+            else
+              repeat
+                nextp := TStatementNode(hp.Right);
+                case hp.left.nodetype of
+                  blockn:
+                    if not Assigned(TBlockNode(hp.left).left) then
+                      begin
+                        { Empty block - delete statement (and block within),
+                          but only if the nf_usercode_entry flag is not set}
+                        if hp.left.flags * [nf_usercode_entry] = [] then
+                          begin
+                            EraseCurrentStatement;
+                            Continue;
+                          end;
+                      end
+                    else
+                      begin
+                        if (TStatementNode(TBlockNode(hp.left).left).left.nodetype = nothingn) and
+                          not Assigned(TStatementNode(TBlockNode(hp.left).left).right) then
+                          begin
+                            { Block contains only a statement->nothingn branch }
+                            EraseCurrentStatement;
+                            Continue;
+                          end;
+
+                        if not Assigned(nextp) then
+                          begin
+                            { If the last statement contains a block, Merge them
+                              (statements within will already be simplified) }
+
+                            { Internal error is triggered if the calling block only
+                              had one statement - code flow should have exited
+                              earlier. }
+                            nextp := TStatementNode(TBlockNode(hp.left).left);
+                            TBlockNode(hp.left).left := nil;
+                            EraseCurrentStatement;
+                            Continue;
+                          end;
+                      end;
+                  nothingn:
+                    { Make sure it's not the only node left }
+                      begin
+                        { Delete statement (and nothing node within) }
+                        EraseCurrentStatement;
+                        Continue;
+                      end;
+                  else
+                    ;
                 end;
-              else
-                ;
-            end;
+
+                lastp := hp;
+                hp := nextp;
+              until not Assigned(hp);
           end;
+
 {$ifdef break_inlining}
         { simple sequence of tempcreate, assign and return temp.? }
         if GetStatements(left,a) and
nothing-node-simplify.patch (6,268 bytes)   

Issue History

Date Modified Username Field Change
2020-12-10 02:46 J. Gareth Moreton New Issue
2020-12-10 02:46 J. Gareth Moreton File Added: nothing-node-simplify.patch
2020-12-10 02:47 J. Gareth Moreton Tag Attached: patch
2020-12-10 02:47 J. Gareth Moreton Tag Attached: compiler
2020-12-10 02:47 J. Gareth Moreton Tag Attached: refactor
2020-12-10 02:47 J. Gareth Moreton Tag Attached: node
2020-12-15 17:34 J. Gareth Moreton Note Added: 0127623
2021-02-02 06:29 J. Gareth Moreton File Deleted: nothing-node-simplify.patch
2021-02-02 06:34 J. Gareth Moreton Note Added: 0128733
2021-02-02 06:34 J. Gareth Moreton File Added: nothing-node-simplify.patch
2021-02-02 22:15 Florian Note Added: 0128747
2021-02-03 10:52 J. Gareth Moreton Note Added: 0128754
2021-02-03 20:08 Florian Note Added: 0128759
2021-02-11 16:31 J. Gareth Moreton Note Added: 0128895
2021-02-11 16:32 J. Gareth Moreton Note Edited: 0128895 View Revisions
2021-02-11 16:33 J. Gareth Moreton Note Edited: 0128895 View Revisions
2021-03-07 11:24 J. Gareth Moreton Note Added: 0129466
2021-03-09 03:09 J. Gareth Moreton File Deleted: nothing-node-simplify.patch
2021-03-09 03:11 J. Gareth Moreton Note Added: 0129522
2021-03-09 03:11 J. Gareth Moreton File Added: nothing-node-simplify.patch
2021-03-25 22:54 Florian Assigned To => Florian
2021-03-25 22:54 Florian Status new => resolved
2021-03-25 22:54 Florian Resolution open => fixed
2021-03-25 22:54 Florian Fixed in Version => 3.3.1
2021-03-25 22:54 Florian Fixed in Revision => 49054
2021-03-25 22:54 Florian FPCTarget => -