Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/Transforms/InstCombine/pr62311.ll | ||
|---|---|---|
| 3 ↗ | (On Diff #526547) | I don't think we should run O3 here. llvm/test/Transforms/PhaseOrdering is the place to check for pass interactions in the default pipeline. |
| 17 ↗ | (On Diff #526547) | nit: remove unneeded local_unnamed_addr #0, dso_local and C++ name mangling for the function name. The test with O3 will likely need an x86 target triple. |
Shall I reduce llvm/test/Transforms/InstCombine/pr62311.ll test case. Or it is fine as it is because instcombine changes can be tested with a smaller test case also?
| llvm/test/Transforms/InstCombine/pr62311.ll | ||
|---|---|---|
| 297 ↗ | (On Diff #526579) | Personally, think all of these test cases are more complex than what we need. Something simple like: define i1 @icmp_eq_m1_and_eq_m1(i8 %x, i8 %y) {
%rx = icmp eq i8 %x, -1
%ry = icmp eq i8 %y, -1
%r = and i1 %rx, %ry
ret i1 %r
}Is probably more inline with what we want. One more thing. Can you please postfix "_fail" to all the negative tests so it clear to the reader. Also can you rename the file to give it a more descriptive name. Maybe something like: |
You have the parent-child relationship of the test/impl patches backwards. This should be the parent of D151660, not the otherway around.
Thanks, updated now, It was a confusion.
| llvm/test/Transforms/InstCombine/pr62311.ll | ||
|---|---|---|
| 297 ↗ | (On Diff #526579) | Thanks, Updated the test cases and put them in and-or-icmps.ll |
Please regenerate the tests here + in the implementation (I think you forgot to when you switched the parent/child relationship).
Yes, I am not sure about how parent/child revisions work.
I will update the patch soon, seems the test cases are not correct for this patch, even without change there is icmp fold happening.
Without change -
define i1 @icmp_eq_m1_and_eq_m1(i8 %x, i8 %y) {
; CHECK-LABEL: @icmp_eq_m1_and_eq_m1(
; CHECK-NEXT: [[TMP1:%.*]] = and i8 [[X:%.*]], [[Y:%.*]]
; CHECK-NEXT: [[R:%.*]] = icmp eq i8 [[TMP1]], -1
; CHECK-NEXT: ret i1 [[R]]
;
%rx = icmp eq i8 %x, -1
%ry = icmp eq i8 %y, -1
%r = and i1 %rx, %ry
ret i1 %r
}No problem. It really is there to represent multiple dependent commits.
So what we want to get submitted to the project is Commit A and Commit B.
Commit A: Tests
Commit B: Implementation
Commit B depends on commit A in the sense that it doesn't apply to head, it has diffs based on commit A.
parent/child is there to represent that relationship.
I will update the patch soon, seems the test cases are not correct for this patch, even without change there is icmp fold happening.
Without change -
define i1 @icmp_eq_m1_and_eq_m1(i8 %x, i8 %y) { ; CHECK-LABEL: @icmp_eq_m1_and_eq_m1( ; CHECK-NEXT: [[TMP1:%.*]] = and i8 [[X:%.*]], [[Y:%.*]] ; CHECK-NEXT: [[R:%.*]] = icmp eq i8 [[TMP1]], -1 ; CHECK-NEXT: ret i1 [[R]] ; %rx = icmp eq i8 %x, -1 %ry = icmp eq i8 %y, -1 %r = and i1 %rx, %ry ret i1 %r }
It seems that only the vector test '@allones' still changes as a result of this patch. This means that
the code is implemented somewhere else but it is not able to handle all types (this is probably
what nikic's comment was about: https://alive2.llvm.org/ce/z/CN7Cgd).
The code currently exists in: foldLogOpOfMaskedICmps (specifically the if (Mask & BMask_AllOnes) { and if (Mask & AMask_AllOnes) { cases).
I'm not sure why it doesn't work for the '@allones' version, it seems get about half the combines, but it fails early.
Can you test your patch on the following codes:
define <2 x i1> @icmp_eq_m1_and_eq_m1(<2 x i8> %x, <2 x i8> %y) {
%rx = icmp eq <2 x i8> %x, <i8 -1, i8 undef>
%ry = icmp eq <2 x i8> %y, <i8 -1, i8 undef>
%r = and <2 x i1> %rx, %ry
ret <2 x i1> %r
}?
That should work with your patch and not without it. If so update your tests with that pattern (partially undef vecs).
Please add tests, however, for partial undef vecs where the undef elements don't match: https://alive2.llvm.org/ce/z/P83Svp
Then please update the todo in nikics code to say "remove this *and below*...."
| llvm/test/Transforms/InstCombine/and-or-icmps.ll | ||
|---|---|---|
| 2498 | Can you add a negative test of this with the following inputs: %rx = icmp eq <2 x i8> %x, <i8 -1, i8 undef> %ry = icmp eq <2 x i8> %y, <i8 undef, i8 -1> and %rx = icmp eq <2 x i8> %x, <i8 undef, i8 undef> %ry = icmp eq <2 x i8> %y, <i8 -1, i8 -2> | |
| 2548 | make rx here -1, -1. | |
Can you add a negative test of this with the following inputs:
and