Skip to content
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

ZeroObj assertions #65257

Merged
merged 1 commit into from
Feb 27, 2022
Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 12, 2022

It is the case that the IR supports the "zero" node for structs in two positions: on the RHS of an assignment (InitBlk form) and under a return, in case the ABI return type is scalar.

Meanwhile, assertion propagation "blindly" replaced structs with zeroes, and so workarounds had to be applied in order for the IR to remain valid, in the form of the NO_CSE flag, applied either explicitly (multi-reg returns) or implicitly (ADDR(LCL) created by impNormStructVal for call args).

This was:

a) A CQ problem in cases where we forgot to clear the NO_CSE flag when the node's parent was updated, say after inlining.
b) A burden for enabling struct LCL_VAR arguments, as one had to remember to mark them NO_CSE in all situations.

This change fixes the problem by deleting propagation of zeroes for local uses, instead propagating them as part of their parents (ASGs and RETURNs).

This has the CQ benefits of not being affected by stale NO_CSEs and the drawback of not participating in the "chained" propagation, where we first copy-propagated something, and then zero-propagated into the new local as well. These cases seem rather rare, so I decided not to spend TP on fixing them by looking at the copy assertions in the new code.

This change also deletes the zero propagation code for SIMDs. It was only useful in cases we had a promoted SIMD field that was zero-inited via an InitBlk on the parent struct. That promotion code was (and is) creating nodes that look like integral constants, except they are of TYP_SIMD. We should delete that form and use proper SIMD zero nodes instead, and design the propagation story for them separately.

I have considered introducing a new flag for this, say GTF_VAR_ZERO_STRUCT_PROP_ALLOWED, to be set on nodes under parents that support "zero" operands, but it seemed like a less robust (if technically "easier") solution to me.

Diffs.

Contributes to #51569.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 12, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 12, 2022
@ghost
Copy link

ghost commented Feb 12, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

It is the case that the IR supports the "zero" node for structs in two positions: on the RHS of an assignment (InitBlk form) and under a return, in case the ABI return type is scalar.

Meanwhile, assertion propagation "blindly" replaced structs with zeroes, and so workarounds had to be applied in order for the IR to remain valid, in the form of the NO_CSE flag, applied either explicitly (multi-reg returns) or implicitly (ADDR(LCL) created by impNormSturctVal for call args).

This was:

a) A CQ problem in cases where we forgot to clear the NO_CSE flag when the node's parent was updated, say after inlining.
b) A burden for enabling struct LCL_VAR arguments, as one had to remembered to mark them NO_CSE in all situation.

This change fixes the problem by deleting propagation of zeroes for local uses, instead propagating them as part of their parents (ASGs and RETURNs).

This has the CQ benefits of not being affected by stale NO_CSEs and the drawback of not participating in the "chained" propagation, where we first copy-propagated something, and then zero-propagated into the new local as well. These cases seem rather rare, so I decided not to spend TP on fixing them by looking at the copy assertions in the new code.

This change also deletes the zero propagation code for SIMDs. It was only useful in cases we had a promoted SIMD field that was zero-inited via an InitBlk on the parent struct. That promotion code was (and is) creating nodes that look like integral constants, except they are of TYP_SIMD. We should delete that form and use proper SIMD zero nodes instead, and design the propagation story for them separately.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

It is the case that the IR supports the "zero" node for structs
in two positions: on the RHS of an assignment (InitBlk form) and
under a return, in case the ABI return type is scalar.

Meanwhile, assertion propagation "blindly" replaced structs with
zeroes, and so workarounds had to be applied in order for the IR
to remain valid, in the form of the NO_CSE flag, applied either
explicitly (multi-reg returns) or implicitly (ADDR(LCL) created by
"impNormSturctVal" for call args).

This was:

a) A CQ problem in cases where we forgot to clear the NO_CSE flag
   when the node's parent was updated, say after inlining.
b) A burden for enabling struct LCL_VAR arguments, as one had to
   remembered to mark them NO_CSE in all situation.

This change fixes the problem by deleting propagation of zeroes for
local uses, instead propagating them as part of their parents (ASGs
and RETURNs).

This has the CQ benefits of not being affected by stale NO_CSEs and
the drawback of not participating in the "chained" propagation, where
we first copy-propagated something, and then zero-propagated into the
new local as well. These cases seem rather rare, so I decided not to
spend TP on fixing them by looking at the copy assertions in the new
code.

This change also deletes the zero propagation code for SIMDs. It was
only useful in cases we had a promoted SIMD field that was zero-inited
via an InitBlk on the parent struct. That promotion code was (and is)
creating nodes that look like integral constants, except they are of
TYP_SIMD. We should delete that form and use proper SIMD zero nodes
instead, and design the propagation story for them separately.
@AndyAyersMS
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member

Arm64 build issue is fixed, so rerunning CI.

@AndyAyersMS
Copy link
Member

CI issues, restarted some things.

@AndyAyersMS AndyAyersMS merged commit 68fb7fc into dotnet:main Feb 27, 2022
@SingleAccretion SingleAccretion deleted the No-Prop-Zero-Struct branch February 27, 2022 16:21
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants