-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow deleting any kind of nodes after address mode formation #77872
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis is a general solution to #77152, allowing for deleting any set of nodes that contributed to an address mode, especially those nodes on the Note that for the #77152 case, this deletes all the Fixes #77152.
|
{ | ||
JITDUMP("Removing unused node:\n "); | ||
DISPNODE(unused); | ||
|
||
BlockRange().Remove(unused); | ||
|
||
if (unused->OperIs(GT_ADD, GT_MUL, GT_LSH)) | ||
for (GenTree* operand : unused->Operands()) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add DEBUG_DESTROY_NODE(unused);
after the for
loop?
@AndyAyersMS Do you think this is the "better" solution over the "targeted" (maybe: hack?) #77871? |
Yes. |
No diffs, about 0.01% throughput regression |
This is a general solution to dotnet#77152, allowing for deleting any set of nodes that contributed to an address mode, especially those nodes on the `op1` side of a GT_MUL with a zero `op2`. Note that for the dotnet#77152 case, this deletes all the `op1` nodes, but leaves behind a dead temp var that was created as part of long MUL rationalization. Fixes dotnet#77152.
b86c8ad
to
867fd64
Compare
This is a general solution to #77152, allowing for deleting any set of nodes that contributed to an address mode, especially those nodes on the
op1
side of a GT_MUL with a zeroop2
.Note that for the #77152 case, this deletes all the
op1
nodes, but leaves behind a dead temp var that was created as part of long MUL rationalization.Fixes #77152.