-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[DAG] visitFREEZE - enable SRA/SRL handling #148252
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
Conversation
@@ -204,7 +204,8 @@ define signext i32 @findLastSet_i32(i32 signext %a) nounwind { | |||
; RV64I-NEXT: add a1, a1, a2 | |||
; RV64I-NEXT: slli a2, a1, 16 | |||
; RV64I-NEXT: add a1, a1, a2 | |||
; RV64I-NEXT: srliw a1, a1, 24 | |||
; RV64I-NEXT: slli a1, a1, 34 |
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.
I spent some time with this yesterday. The issue is that we don't push the freeze up the pairs of slli+add. I was able to trick it a bit by emitting a freeze before the slli+add pairs are emitted in expandCTPOP.
; RV64I-NEXT: or a0, a0, a2 | ||
; RV64I-NEXT: srliw a2, a0, 8 | ||
; RV64I-NEXT: slli a2, a0, 33 |
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.
I think the issue here is that we moved a freeze in the entry block which allowed computeKnownBits to compute a value for the output of the block. Then an AssertZExt was emitted in the the cond.false block. This allowed us to remove some bits from the AND mask and our isel code for srliw doesn't use computeKnownBits to fill in missing bits.
✅ With the latest revision this PR passed the C/C++ code formatter. |
Closing - handling this as part of #152107 reduces the regressions we need to handle |
WIP - still investigating the regressions first reported on https://reviews.llvm.org/D136529
Fixes #150204