[LiveRangeEdit] Remove any MemoryOperand on MI when converting it to KILL. #114407
Conversation
|
@llvm/pr-subscribers-llvm-regalloc @llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesAn optimization pass may change an instruction into a KILL it seems when there is no use for the produced value. In this case the register coalescer did so while leaving the MMO untouched. The MachineVerifier did not accept this, but it seems reasonable that it should. This patch makes the machine verifier accept dangling MMOs on KILL instructions. The alternative might be to have the actor always call something like MI->removeAsKill() which also removed the MMO after setting the opcode to KILL. @arsenm @ellishg @topperc @michaelmaitland Full diff: https://github.com/llvm/llvm-project/pull/114407.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index e2c09fe25d55cd..cc5acccdc8df66 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -2271,7 +2271,7 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
// Check the MachineMemOperands for basic consistency.
for (MachineMemOperand *Op : MI->memoperands()) {
- if (Op->isLoad() && !MI->mayLoad())
+ if (Op->isLoad() && (!MI->mayLoad() && !MI->isKill()))
report("Missing mayLoad flag", MI);
if (Op->isStore() && !MI->mayStore())
report("Missing mayStore flag", MI);
diff --git a/llvm/test/CodeGen/SystemZ/machine-ver-kill-memop.ll b/llvm/test/CodeGen/SystemZ/machine-ver-kill-memop.ll
new file mode 100644
index 00000000000000..b3cbc332f473a3
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/machine-ver-kill-memop.ll
@@ -0,0 +1,30 @@
+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z16 -disable-machine-dce \
+; RUN: -verify-machineinstrs -O3
+;
+; Test that this passes the verifier even though a KILL instruction with a
+; dangling memory operand emerges.
+
+define void @fun(ptr %Src, ptr %Dst) {
+ br label %2
+
+2:
+ %3 = load i32, ptr %Src, align 4
+ %4 = freeze i32 %3
+ %5 = icmp eq i32 %4, 0
+ %6 = load i32, ptr poison, align 4
+ %7 = select i1 %5, i32 0, i32 %6
+ %8 = load i32, ptr %Src, align 4
+ %9 = freeze i32 %8
+ %10 = icmp eq i32 %9, 0
+ %11 = load i32, ptr poison, align 4
+ %12 = select i1 %10, i32 0, i32 %11
+ br label %13
+
+13:
+ %14 = phi i32 [ %12, %13 ], [ %7, %2 ]
+ %15 = icmp slt i32 %14, 5
+ %16 = zext i1 %15 to i64
+ %17 = xor i64 poison, %16
+ store i64 %17, ptr %Dst, align 8
+ br label %13
+}
|
It should just delete the memory operands. There's no reason to accept this |
|
Updated per review.
ok - I thought it was ok to make things simple for the CodeGen passes and just let them switch to KILL opcode, but I see now that they actually take care to remove operands and MMOs. |
| alignment: 16 | ||
| tracksRegLiveness: true | ||
| noPhis: true | ||
| isSSA: false | ||
| noVRegs: false | ||
| hasFakeUses: false | ||
| registers: | ||
| - { id: 0, class: grx32bit } | ||
| - { id: 1, class: grx32bit } | ||
| - { id: 2, class: grx32bit } | ||
| - { id: 3, class: addr64bit } | ||
| - { id: 4, class: gr64bit } | ||
| - { id: 5, class: grx32bit } | ||
| - { id: 6, class: grx32bit } | ||
| - { id: 7, class: grx32bit } | ||
| - { id: 8, class: addr64bit } | ||
| liveins: | ||
| - { reg: '$r2d', virtual-reg: '%3' } | ||
| frameInfo: | ||
| maxAlignment: 1 | ||
| machineFunctionInfo: {} |
There was a problem hiding this comment.
| alignment: 16 | |
| tracksRegLiveness: true | |
| noPhis: true | |
| isSSA: false | |
| noVRegs: false | |
| hasFakeUses: false | |
| registers: | |
| - { id: 0, class: grx32bit } | |
| - { id: 1, class: grx32bit } | |
| - { id: 2, class: grx32bit } | |
| - { id: 3, class: addr64bit } | |
| - { id: 4, class: gr64bit } | |
| - { id: 5, class: grx32bit } | |
| - { id: 6, class: grx32bit } | |
| - { id: 7, class: grx32bit } | |
| - { id: 8, class: addr64bit } | |
| liveins: | |
| - { reg: '$r2d', virtual-reg: '%3' } | |
| frameInfo: | |
| maxAlignment: 1 | |
| machineFunctionInfo: {} | |
| tracksRegLiveness: true |
There was a problem hiding this comment.
actually, that line is already present further up...
There was a problem hiding this comment.
This is to delete everything else, not add the line
|
test updated |
|
One more try with the test :) |
JonPsson1
changed the title
I rewrote it here now, and I will write a better commit message as well when I merge it. |
|
Committed - thanks for review! |
An optimization pass may change an instruction into a KILL it seems when there is no use for the produced value. In this case the register coalescer did so while leaving the MMO untouched. The MachineVerifier did not accept this, but it seems reasonable that it should.
This patch makes the machine verifier accept dangling MMOs on KILL instructions.
The alternative might be to have the actor always call something like MI->removeAsKill() which also removed the MMO after setting the opcode to KILL.
@arsenm @ellishg @topperc @michaelmaitland