Refactor RyuJIT/x86 long decomposition code #6202
Conversation
|
@adiaaida @CarolEidt @mikedn PTAL |
| { | ||
| for (GenTree* stmt = block->bbTreeList; stmt != nullptr; stmt = stmt->gtNext) | ||
| { | ||
| if (stmt->gtFlags & GTF_STMT_SKIP_LOWER) |
There was a problem hiding this comment.
Side note: this is incorrect. Should be fixed when we deal with duplicate decomposition caused by embedded temporary creation logic.
|
Looks great. Though I would have called it |
| if (!gtLong->gtOp.gtOp2->OperIsLeaf()) | ||
| { | ||
| // REVIEW: should we also use CreateEmbeddedFormTemp() here? | ||
| GenTreeStmt* dataHighStmt = comp->fgInsertEmbeddedFormTemp(>Long->gtOp.gtOp2); |
There was a problem hiding this comment.
should we also use CreateEmbeddedFormTemp() here?
IMO yes. As explained in the GT_NEG PR that logic isn't necessary here but I don't think it helps to avoid using CreateEmbeddedFormTemp, quite the contrary.
I think I'll rename the class (and files) to DecomposeLongs to be more explicit. |
BruceForstall
force-pushed
the
RefactorDecomp
branch
from
57cb5ad to
d7a688f
Compare
All decomposition code is now in decompose.cpp, in a Decompose class. Each node type that is decomposed has its own member function to do the decomposition. Various helpers have been added to reduce code duplication. In general, though, the code is as it was before.
BruceForstall
force-pushed
the
RefactorDecomp
branch
from
d7a688f to
0e5227d
Compare
|
@mmitche the Ubuntu x64 checked run failed with |
|
@dotnet-bot test Ubuntu x64 Checked Build and Test |
| DecomposeStoreInd(ppTree, data); | ||
| break; | ||
|
|
||
| case GT_STORE_LCL_FLD: |
There was a problem hiding this comment.
Can we put these in their own functions too, so that when we fill them in, we just fill out the function? (here, GT_IND, for sure. Not sure on the arithmetic operations, because those may be morphed into helper calls in morph rather than handled in Decomp, like some div/mul operations already are).
There was a problem hiding this comment.
I assume you're referring to the NYI. Yes, they should be put in their own functions when filled in. Since i didn't know if they would share functions or how they would be structured, I didn't want to waste time on creating function shells now.
There was a problem hiding this comment.
Yes, that's what I meant. And that makes sense.
|
Otherwise, looks good to me. |
|
@BruceForstall Because the Linux/OSX runs require multiple jobs, the top job fails if any subjob fails. This failure analysis ensures that we don't end up with a bunch of unknown failures. If you look at the page, you'll see the _tst sub project failed. Clicking on that, I see a test failure in the PAL tests: http://dotnet-ci.cloudapp.net/job/dotnet_coreclr/job/master/job/checked_ubuntu_flow_prtest/3897/ |
Refactor RyuJIT/x86 long decomposition code Commit migrated from dotnet/coreclr@c99eaa4
All decomposition code is now in decompose.cpp, in a Decompose class.
Each node type that is decomposed has its own member function to do
the decomposition. Various helpers have been added to reduce code
duplication. In general, though, the code is as it was before.
Fixes #5866