Decomposing 64-bit storeind instruction to 32-bit storeind instructions#5380
Conversation
|
Opened for testing |
|
@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test |
|
@dotnet-bot test this |
|
@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test |
sejongoh
force-pushed
the
x86_decompose_storeind
branch
from
d5b0906 to
ef9a8dc
Compare
|
@dotnet-bot test Windows_NT x86 ryujit Checked Build and Test |
|
@dotnet-bot test Ubuntu x64 Checked Build and Test |
sejongoh
force-pushed
the
x86_decompose_storeind
branch
3 times, most recently
from
1960742 to
ccddd11
Compare
|
@BruceForstall PTAL. |
| } | ||
|
|
||
| // Internal links of storeIndHigh tree | ||
| dataHigh->gtNext = addrBaseHigh; |
There was a problem hiding this comment.
Can't we use SimpleLinkNodeAfter or something similar? This kind of manual linking hurts the readability of the code.
There was a problem hiding this comment.
I would second the suggestion (and apologies for my delay, as I'm out of office this week).
|
Side note not really related to this PR: IMO it would be better to pull decomposition stuff in a separate class in a separate file. JIT already suffers a lot from huge classes and files. |
|
@mikedn - I like the idea of pulling out the decomposition code. |
sejongoh
force-pushed
the
x86_decompose_storeind
branch
from
ccddd11 to
5341479
Compare
|
@CarolEidt @mikedn I will open an issue for pulling out the decomposition code. |
|
@mikedn @CarolEidt PTAL |
| comp->gtDispTree(dataHighStmt); | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Doesn't dataHighStmt need the same workaround for embedded statements?
There was a problem hiding this comment.
I think that dataHighStmt is not a new statement as long as gtLong->gtOp.gtOp2 comes after gtLong->gtOp.gtOp1 in the linear order. Do you think that we could have the REVERSE flag on gtLong node? If so, we need the same workaround here.
There was a problem hiding this comment.
Right, without GTF_REVERSE_OPS dataHighStmt should never become a top-level statement, the GT_LCL_VAR node produced by dataLowStmt prevents that. I don't see any code setting GTF_REVERSE_OPS on GT_LONG nodes and hopefully no such code will ever be added as it would simply complicate things more. So not applying the workaround here seems the right thing to do.
|
I don't think I mentioned that this probably won't generate very good code, when I played with decomposing GT_STOREIND I ended up with something like due to the address temporary. Anyway, IMO this is good enough to be merged. CQ issues and other implementation issues such as redundancy of the workaround code can be addressed later when more decomposition work is done and we hopefully get a better picture of the whole thing. The embedded statement stuff seems a bit nightmarish and I still wonder if switching to linear IR wouldn't be easier. |
|
@mikedn @CarolEidt Thanks for thorough review! What we want here is simply breaking a store into two stores. IMO, IR manipulation is quite complicated and hope that we can improve this. |
sejongoh
force-pushed
the
x86_decompose_storeind
branch
from
5341479 to
15c8b69
Compare
sejongoh
force-pushed
the
x86_decompose_storeind
branch
from
15c8b69 to
780400f
Compare
|
@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test |
|
@dotnet-bot test Ubuntu x64 Checked Build and Test |
|
@dotnet-bot test OSX x64 Checked Build and Test |
…oreind Decomposing 64-bit storeind instruction to 32-bit storeind instructions Commit migrated from dotnet/coreclr@b464dc3
4 participants
Decompose 64-bit storeind instruction.