[TwoAddressInstruction] Use isPlainlyKilled in processTiedPairs #65976
Conversation
| ; CHECK-LIS-NEXT: pshufd {{.*#+}} xmm0 = xmm3[1,0,1,1] | ||
| ; CHECK-LIS-NEXT: pblendw {{.*#+}} xmm2 = xmm1[0,1],xmm2[2,3,4,5,6,7] | ||
| ; CHECK-LIS-NEXT: por %xmm0, %xmm2 | ||
| ; CHECK-LIS-NEXT: movdqa %xmm2, %xmm0 |
There was a problem hiding this comment.
Regression here but I don't think TwoAddressInstruction is doing anything wrong. I can see RegisterCoalescer rematerializing a V_SET0 in a slightly unfortunate place which causes overlapping lifetimes for (a) one value that wants to use $xmm0 on input and (b) another value that wants to use $xmm0 on return, so one of them has to pick a different register and that causes the extra COPY.
There was a problem hiding this comment.
We have similar issues with multiple zero vector registers, so normally I'd say this is acceptable but I'm worried this change might be encouraging the underlying problem?
There was a problem hiding this comment.
I saw equal and opposite changes of this sort in combine-or.ll and combine-rotates.ll so I suspect it is neither better nor worse, but I'm not sure how to prove that.
llvm/test/CodeGen/SystemZ/rot-02.ll
Outdated
| @@ -2,7 +2,8 @@ | |||
| ; Test removal of AND operations that don't affect last 6 bits of rotate amount | |||
| ; operand. | |||
| ; | |||
| ; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s | |||
| ; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s -check-prefixes=CHECK,CHECK-LV | |||
| ; RUN: llc < %s -mtriple=s390x-linux-gnu -early-live-intervals | FileCheck %s -check-prefixes=CHECK,CHECK-LIS | |||
There was a problem hiding this comment.
These changes look redundant?
There was a problem hiding this comment.
Ugh. I am still struggling with how to deal with stacked patches. In the first commit the separate prefixes are necessary. In the second patch, codegen improves and they are no longer needed.
| @@ -1,5 +1,6 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py | |||
| ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=CHECK,SSE2 | |||
| ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 | FileCheck %s --check-prefixes=CHECK,SSE2,SSE2-LV | |||
| ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+sse2 -early-live-intervals | FileCheck %s --check-prefixes=CHECK,SSE2,SSE2-LIS | |||
| ; CHECK-LIS-NEXT: pshufd {{.*#+}} xmm0 = xmm3[1,0,1,1] | ||
| ; CHECK-LIS-NEXT: pblendw {{.*#+}} xmm2 = xmm1[0,1],xmm2[2,3,4,5,6,7] | ||
| ; CHECK-LIS-NEXT: por %xmm0, %xmm2 | ||
| ; CHECK-LIS-NEXT: movdqa %xmm2, %xmm0 |
There was a problem hiding this comment.
We have similar issues with multiple zero vector registers, so normally I'd say this is acceptable but I'm worried this change might be encouraging the underlying problem?
I will try to split this into two separate PRs, one for each commit. Please bear with me... |
jayfoad
changed the title
|
The first commit (adding test coverage) is now #66058. For this PR, please only review the second commit. |
|
Rebased. Ping! |
| @@ -477,3 +451,6 @@ declare i5 @llvm.fshl.i5(i5, i5, i5) | |||
|
|
|||
| declare <4 x i32> @llvm.fshl.v4i32(<4 x i32>, <4 x i32>, <4 x i32>) | |||
| declare <4 x i32> @llvm.fshr.v4i32(<4 x i32>, <4 x i32>, <4 x i32>) | |||
| ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line: | |||
| ; SSE2-LIS: {{.*}} | |||
| ; SSE2-LV: {{.*}} | |||
There was a problem hiding this comment.
Remove these prefixes from the RUN lines?
Calling isPlainlyKilled instead of directly checking for a kill flag should make processTiedPairs behave the same with LiveIntervals (i.e. when compiling with -early-live-intervals) as it does with LiveVariables.
Calling isPlainlyKilled instead of directly checking for a kill flag
should make processTiedPairs behave the same with LiveIntervals
(i.e. when compiling with -early-live-intervals) as it does with
LiveVariables.