-
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
JIT: Fold "shift-by-zero" in lower #61222
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsFixes a Checked-only assert found in #61209 by Fuzzlyn. cc @jakobbotsch
|
@@ -5773,7 +5783,7 @@ void Lowering::LowerShift(GenTreeOp* shift) | |||
assert(!cast->CastOp()->isContained()); | |||
|
|||
// It has to be an upcast and CNS must be in [1..srcBits) range | |||
if ((srcBits < dstBits) && ((UINT32)cns->IconValue() < srcBits)) | |||
if ((srcBits < dstBits) && (cns->IconValue() > 0) && (cns->IconValue() < srcBits)) |
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.
Do I understand correctly this is the fix and the removal of nop shifts is a separate optimization, or is that required as well?
FWIW, I would prefer we not do these ad-hoc opts in lowering (instead tracing back the where the frontend "failed at its job" and so on).
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.
@SingleAccretion it's pretty normal to duplicate peepholes accross phases and we already do it. Front-end didn't fail here, only rationalizer managed to fold this:
N036 ( 47, 59) [000024] -ACXG------- \--* LSH int <l:$2c8, c:$2d2>
N035 ( 2, 5) [000067] -A--G------- \--* COMMA int
N033 ( 1, 3) [000042] -A--G---R--- +--* ASG ref <l:$1c2, c:$1d5>
N032 ( 1, 1) [000041] D------N---- | +--* LCL_VAR ref V02 tmp2 d:1 <l:$280, c:$85>
N031 ( 1, 1) [000060] ------------ | \--* LCL_VAR ref V03 cse0 u:1 <l:$280, c:$81>
N034 ( 1, 2) [000066] ------------ \--* CNS_INT int 0 <l:$2d1, c:$2d0>
into
N033 ( 1, 3) [000042] DA--G------- * STORE_LCL_VAR ref V02 tmp2 d:1
N034 ( 1, 2) [000066] ------------ t66 = CNS_INT int 0 <l:$2d1, c:$2d0>
/--* t9 int
+--* t66 int
N036 ( 47, 59) [000024] ---XG------- t24 = * LSH int <l:$2c8, c:$2d2>
/--* t24 int
N037 ( 48, 60) [000025] ---XG------- * RETURN int $107
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 some day we'll get a DSL for transformations which will be compiled into morph and lower at the same time (that's how some C++ compilers work today)
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.
Is it really worth to add this optimization in lower that only has hits in fuzzer generated code?
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.
@jakobbotsch not really, but the diff found 4 more cases and we still don't have collections for real-world apps to judge. but let me remove it since it raised concerns by you two
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.
PTAL
Fixes a Checked-only assert found in #61209 by Fuzzlyn.
cc @jakobbotsch
A few diffs:
e.g. https://www.diffchecker.com/oEoe5xQk