C++, C# IR: Stabilize sort order for basic blocks.#4604
C++, C# IR: Stabilize sort order for basic blocks.#4604rdmarsh2 merged 3 commits intogithub:mainfrom
Conversation
criemen
force-pushed
the
ir-block-sort-order
branch
from
2eedcfd to
78d885e
Compare
rdmarsh2
left a comment
rdmarsh2
left a comment
There was a problem hiding this comment.
Looks good to me. In addition to being more stable, the new ordering is also more readable.
I have left a few notes about strange block orders for posterity, but I think they're relatively small issues and somewhat complicated to fix.
| #-----| False -> Block 1 | ||
| #-----| True -> Block 4 | ||
|
|
||
| # 477| Block 1 | ||
| # 477| r477_4(glval<bool>) = VariableAddress[b] : | ||
| # 477| r477_5(bool) = Load[b] : &:r477_4, ~m? | ||
| # 477| v477_6(void) = ConditionalBranch : r477_5 | ||
| #-----| False -> Block 10 | ||
| #-----| True -> Block 12 | ||
| # 477| r477_4(glval<bool>) = VariableAddress[#temp477:9] : | ||
| # 477| r477_5(bool) = Constant[0] : | ||
| # 477| mu477_6(bool) = Store[#temp477:9] : &:r477_4, r477_5 | ||
| #-----| Goto -> Block 2 | ||
|
|
||
| # 478| Block 2 | ||
| # 478| r478_1(glval<bool>) = VariableAddress[#temp478:9] : | ||
| # 478| r478_2(bool) = Constant[0] : | ||
| # 478| mu478_3(bool) = Store[#temp478:9] : &:r478_1, r478_2 | ||
| #-----| Goto -> Block 3 | ||
| # 477| Block 2 | ||
| # 477| r477_7(glval<bool>) = VariableAddress[#temp477:9] : | ||
| # 477| r477_8(bool) = Load[#temp477:9] : &:r477_7, ~m? | ||
| # 477| r477_9(glval<bool>) = VariableAddress[x] : | ||
| # 477| mu477_10(bool) = Store[x] : &:r477_9, r477_8 | ||
| # 478| r478_1(glval<bool>) = VariableAddress[a] : | ||
| # 478| r478_2(bool) = Load[a] : &:r478_1, ~m? | ||
| # 478| v478_3(void) = ConditionalBranch : r478_2 | ||
| #-----| False -> Block 8 | ||
| #-----| True -> Block 7 | ||
|
|
||
| # 477| Block 3 | ||
| # 477| r477_11(glval<bool>) = VariableAddress[#temp477:9] : | ||
| # 477| r477_12(bool) = Constant[1] : | ||
| # 477| mu477_13(bool) = Store[#temp477:9] : &:r477_11, r477_12 | ||
| #-----| Goto -> Block 2 | ||
|
|
||
| # 477| Block 4 | ||
| # 477| r477_14(glval<bool>) = VariableAddress[b] : | ||
| # 477| r477_15(bool) = Load[b] : &:r477_14, ~m? | ||
| # 477| v477_16(void) = ConditionalBranch : r477_15 | ||
| #-----| False -> Block 1 | ||
| #-----| True -> Block 3 |
There was a problem hiding this comment.
This ordering is a bit strange. It looks like the blocks that compute the result of a short-circuited boolean operation come before the block that tests the right-hand side of the expression. TranslatedElement.getUniqueId is a preorder, but I think this would work better with a postorder. A postorder would be worse for catch and switch statements, though.
| #-----| Case[0] -> Block 2 | ||
| #-----| Case[1] -> Block 3 | ||
| #-----| Default -> Block 4 | ||
|
|
||
| # 560| Block 1 | ||
| # 560| r560_6(glval<int>) = VariableAddress[#return] : | ||
| # 560| v560_7(void) = ReturnValue : &:r560_6, ~m? | ||
| # 560| v560_8(void) = AliasedUse : ~m? | ||
| # 560| v560_9(void) = ExitFunction : |
There was a problem hiding this comment.
This also looks a bit strange - I think the common return block is always Block 1 if there are multiple return statements (or a void return statement in a conditional and a return by fallthrough), but the return instructions will be tacked onto another block if there is only one return statement.
2 participants
Fixes https://github.com/github/codeql-c-analysis-team/issues/81
This PR addresses the issue that basic blocks are not printed in a stable sort order, so small changes in code can lead to big changes in
PrintIRoutput.Implementation caveat: Ideally we could get rid of/deprecate the predicate
getUniqueIdon Instructions, but I did not see a good way how to do that while keeping a sensible order for phi nodes.