View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0038194 | FPC | Compiler | public | 2020-12-10 02:46 | 2021-03-25 22:54 |
Reporter | J. Gareth Moreton | Assigned To | Florian | ||
Priority | normal | Severity | minor | Reproducibility | N/A |
Status | resolved | Resolution | fixed | ||
Platform | Cross-platform | OS | Microsoft Windows | ||
Product Version | 3.3.1 | ||||
Fixed in Version | 3.3.1 | ||||
Summary | 0038194: [Patch / Refactor] Nothing (NOP) node optimisation | ||||
Description | With 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 Reproduce | Apply patch and comfirm identical generated binaries but cleaner node trees and possible speed boost. | ||||
Additional Information | When 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. | ||||
Tags | compiler, node, patch, refactor | ||||
Fixed in Revision | 49054 | ||||
FPCOldBugId | |||||
FPCTarget | - | ||||
Attached Files |
|
|
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). |
|
Brought the patch up to date and fixed a possible problem should "break_inlining" be defined. |
|
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 |
|
Curious. I'll do some investigating. This is x86_64-linux presumably. |
|
Yes, x86_64-linux. |
|
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. |
|
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! |
|
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 |
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 | => - |