JIT: Add Statement::m_treeListEnd #81031
Conversation
Before this change statements have only one field for the "tree list" even though bidirectional iteration is supported after nodes have been linked. This worked fine before the locals tree list existed since the "root node" was always the last node. This does not work for the locals tree list since the "root node" is not part of the list, meaning that we need somewhere else to store the 'last' node. Before this PR the assumption was that the root node was _never_ part of the locals linked list, so we could use the gtNext/gtPrev fields of the root node. However, the added test case shows that in fact it is possible we end up with a top level local. This PR fixes the problem by adding a Statement::m_treeListEnd field that can keep the last node of the locals linked list. While this takes some memory, it seems like the most maintainable way to resolve the problem. I experimented with making the linked list circular or by allocating a new stand-in node when necessary but eventually I ended up here. Fix dotnet#81018
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsBefore this change statements have only one field for the "tree list" This does not work for the locals tree list since the "root node" is not This PR fixes the problem by adding a Statement::m_treeListEnd field Fix #81018 Memory use diff: https://www.diffchecker.com/S1dWNPBV/
|
|
Of course the alternative is to change upstream to stop producing the top level local nodes. In this particular case the culprit is inlining, which ends up with Inserting the inline return expression
[000078] n--X------- ▌ OBJ struct<S0, 1>
[000077] ----------- └──▌ LCL_VAR_ADDR byref V03 loc3
...
STMT00012 ( ??? ... ??? )
[000078] n--X------- ▌ IND byte
[000077] ----------- └──▌ LCL_VAR_ADDR byref V03 loc3
that is folded by local morph into the top-level local. But I'm not sure it's an invariant we want to enforce given that it doesn't exist today. |
Nominally, top-level local access should be transformed into a |
LocalAddressVisitor visiting statement:
STMT00012 ( ??? ... ??? )
[000078] n--X------- ▌ IND byte
[000077] ----------- └──▌ LCL_VAR_ADDR byref V03 loc3
└──▌ byte V03.S0:F0 (offs=0x00) -> V12 tmp8
Replacing the field in promoted struct with local var V12
LocalAddressVisitor modified statement:
STMT00012 ( ??? ... ??? )
[000078] ----------- ▌ LCL_VAR byte V12 tmp8 So maybe just fix this instead if there is prior art in local morph already to get rid of these? I do expect that I may introduce nodes with memory havoc effects into this linked list at some point, at which point I will probably need this change anyway, but I could delay it until then. |
|
I see, the local gets transformed before the indir transform, and we don't look at pre-existing local nodes.
If this will be needed anyway at some point, I suppose there is not much point in delaying things (it will be more code in the end). |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
Before this change statements have only one field for the "tree list"
even though bidirectional iteration is supported after nodes have been
linked. This worked fine before the locals tree list existed since the
"root node" was always the last node.
This does not work for the locals tree list since the "root node" is not
part of the list, meaning that we need somewhere else to store the
'last' node. Before this PR the assumption was that the root node was
never part of the locals linked list, so we could use the
gtNext/gtPrev fields of the root node. However, the added test case
shows that in fact it is possible we end up with a top level local.
This PR fixes the problem by adding a Statement::m_treeListEnd field
that can keep the last node of the locals linked list. While this takes
some memory, it seems like the most maintainable way to resolve the
problem. I experimented with making the linked list circular or by
allocating a new stand-in node when necessary but eventually I ended up
here.
Fix #81018
Memory use diff: https://www.diffchecker.com/S1dWNPBV/
Around 0.17% more memory used for SPMI over libraries.pmi.