-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[X86] Add baseline test for X86 conditional load/store optimization bug #163354
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
|
@llvm/pr-subscribers-backend-x86 Author: None (azwolski) ChangesThis PR adds a baseline test that exposes a bug in the current Without any assumptions about // res, flags2 = sub 0, (and (xor X, -1), Y)
// cload/cstore ..., cond_ne, flag2
// ->
// res, flags2 = sub 0, (and X, Y)
// cload/cstore ..., cond_e, flag2In the provided test case, the value in Fix: #163353 Full diff: https://github.com/llvm/llvm-project/pull/163354.diff 1 Files Affected:
diff --git a/llvm/test/CodeGen/X86/apx/cf.ll b/llvm/test/CodeGen/X86/apx/cf.ll
index b2651e91134ee..4cce2226a91fb 100644
--- a/llvm/test/CodeGen/X86/apx/cf.ll
+++ b/llvm/test/CodeGen/X86/apx/cf.ll
@@ -230,6 +230,24 @@ entry:
ret void
}
+define void @and_cond(i32 %a, i1 %b) {
+; CHECK-LABEL: and_cond:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: testl %edi, %edi
+; CHECK-NEXT: setg %al
+; CHECK-NEXT: xorl %ecx, %ecx
+; CHECK-NEXT: testb %al, %sil
+; CHECK-NEXT: cfcmovel %ecx, 0
+; CHECK-NEXT: retq
+entry:
+ %0 = icmp sgt i32 %a, 0
+ %1 = xor i1 %b, true
+ %3 = and i1 %1, %0
+ %4 = insertelement <1 x i1> zeroinitializer, i1 %3, i64 0
+ call void @llvm.masked.store.v1i32.p0(<1 x i32> zeroinitializer, ptr null, i32 1, <1 x i1> %4)
+ ret void
+}
+
define i64 @redundant_test(i64 %num, ptr %p1, i64 %in) {
; CHECK-LABEL: redundant_test:
; CHECK: # %bb.0:
|
phoebewang
left a comment
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.
LGTM.
RKSimon
left a comment
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.
LGTM - cheers!
…erands (#163353) This PR fixes a bug in combineX86CloadCstore where an optimization was being applied too broadly, causing incorrect code generation. Without any assumptions about `X` this transformation is only valid when `Y` is a non zero power of two/single-bit mask. ```cpp // res, flags2 = sub 0, (and (xor X, -1), Y) // cload/cstore ..., cond_ne, flag2 // -> // res, flags2 = sub 0, (and X, Y) // cload/cstore ..., cond_e, flag2 ``` We can restrict the optimization to most important case, so only apply when `llvm::isOneConstant(Op1.getOperand(1))`. It might be not trivial to find code that creates a SelectionDag with other values of `Y`. Basline test: #163354
…constant operands (#163353)
This PR fixes a bug in combineX86CloadCstore where an optimization was
being applied too broadly, causing incorrect code generation.
Without any assumptions about `X` this transformation is only valid when
`Y` is a non zero power of two/single-bit mask.
```cpp
// res, flags2 = sub 0, (and (xor X, -1), Y)
// cload/cstore ..., cond_ne, flag2
// ->
// res, flags2 = sub 0, (and X, Y)
// cload/cstore ..., cond_e, flag2
```
We can restrict the optimization to most important case, so only apply
when `llvm::isOneConstant(Op1.getOperand(1))`. It might be not trivial
to find code that creates a SelectionDag with other values of `Y`.
Basline test: llvm/llvm-project#163354
This PR adds a baseline test that exposes a bug in the current
combineX86CloadCstoreoptimization. The generated assembly demonstrates incorrect behavior when the optimization is applied without proper constraints.Without any assumptions about
Xthis transformation is only valid whenYis a non zero power of two/single-bit mask.In the provided test case, the value in
%alis unknown at compile time. If%alcontains0, the optimization cannot be applied, because(and (xor X, -1), 0)is not equal to(and X, 0).Fix: #163353