[llvm] [AMDGPU] SIInstrInfo: Fix resultDependsOnExec for VOPC instructions (PR #134629)
Frederik Harwath via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 8 05:01:02 PDT 2025
https://github.com/frederik-h updated https://github.com/llvm/llvm-project/pull/134629
>From f2c235652220c74e0b1419813f6cace8b36a6ede Mon Sep 17 00:00:00 2001
From: Frederik Harwath <fharwath at amd.com>
Date: Mon, 7 Apr 2025 08:41:33 -0400
Subject: [PATCH 1/6] [AMDGPU] SIInstrInfo: Fix resultDependsOnExec for VOPC
instructions
SIInstrInfo::resultDependsOnExec assumes that operand 0 of a
comparison is always the destination of the instruction.
This is not true for instructions in VOPC form where it is
"src0".
Additional unrelated change: Fix spelling mistakes in doc comment.
---
llvm/lib/CodeGen/MachineCSE.cpp | 8 +++----
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 3 +++
.../AMDGPU/si-instr-info-vopc-no-exec.mir | 23 +++++++++++++++++++
3 files changed, 30 insertions(+), 4 deletions(-)
create mode 100644 llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir
diff --git a/llvm/lib/CodeGen/MachineCSE.cpp b/llvm/lib/CodeGen/MachineCSE.cpp
index 6d14509c5934f..1ebf7b96a3b94 100644
--- a/llvm/lib/CodeGen/MachineCSE.cpp
+++ b/llvm/lib/CodeGen/MachineCSE.cpp
@@ -278,10 +278,10 @@ static bool isCallerPreservedOrConstPhysReg(MCRegister Reg,
(MRI.reservedRegsFrozen() && MRI.isConstantPhysReg(Reg));
}
-/// hasLivePhysRegDefUses - Return true if the specified instruction read/write
-/// physical registers (except for dead defs of physical registers). It also
-/// returns the physical register def by reference if it's the only one and the
-/// instruction does not uses a physical register.
+/// hasLivePhysRegDefUses - Return true if the specified instruction
+/// reads/writes physical registers (except for dead defs of physical
+/// registers). It also returns the physical register def by reference if it's
+/// the only one and the instruction does not use a physical register.
bool MachineCSEImpl::hasLivePhysRegDefUses(const MachineInstr *MI,
const MachineBasicBlock *MBB,
SmallSet<MCRegister, 8> &PhysRefs,
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 61fda0eef6314..4990c9a74f6ee 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -151,6 +151,9 @@ static bool resultDependsOnExec(const MachineInstr &MI) {
// Ignore comparisons which are only used masked with exec.
// This allows some hoisting/sinking of VALU comparisons.
if (MI.isCompare()) {
+ if (SIInstrInfo::isVOPC(MI))
+ return false;
+
const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
Register DstReg = MI.getOperand(0).getReg();
if (!DstReg.isVirtual())
diff --git a/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir b/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir
new file mode 100644
index 0000000000000..f1e9447dbd349
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir
@@ -0,0 +1,23 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -run-pass=machine-cse %s -o - | FileCheck %s
+
+---
+name: vopc_comparisons_do_not_depend_on_exec
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; CHECK-LABEL: name: vopc_comparisons_do_not_depend_on_exec
+ ; CHECK: [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
+ ; CHECK-NEXT: V_CMP_EQ_U32_e32 1, killed undef [[DEF]], implicit-def $vcc_lo, implicit $exec
+ ; CHECK-NEXT: [[V_CNDMASK_B32_e32_:%[0-9]+]]:vgpr_32 = V_CNDMASK_B32_e32 16256, undef [[DEF]], implicit $vcc_lo, implicit $exec
+ ; CHECK-NEXT: [[V_LSHLREV_B32_e64_:%[0-9]+]]:vgpr_32 = V_LSHLREV_B32_e64 16, undef [[V_CNDMASK_B32_e32_]], implicit $exec
+ ; CHECK-NEXT: SI_RETURN
+ %0:vgpr_32 = IMPLICIT_DEF
+ V_CMP_EQ_U32_e32 1, killed undef %0, implicit-def $vcc, implicit $exec
+ %1:vgpr_32 = V_CNDMASK_B32_e32 16256, undef %0, implicit $vcc, implicit $exec
+ V_CMP_EQ_U32_e32 1, killed undef %0, implicit-def $vcc, implicit $exec
+ %2:vgpr_32 = V_CNDMASK_B32_e32 16256, undef %0, implicit $vcc, implicit $exec
+ %3:vgpr_32 = V_LSHLREV_B32_e64 16, killed undef %2, implicit $exec
+ %4:vgpr_32 = V_LSHLREV_B32_e64 16, killed undef %1, implicit $exec
+ SI_RETURN
+...
>From b4820bc90b17db75635f4810c8622b2f7c3626bd Mon Sep 17 00:00:00 2001
From: Frederik Harwath <fharwath at amd.com>
Date: Mon, 7 Apr 2025 10:15:28 -0400
Subject: [PATCH 2/6] Review changes
---
llvm/lib/CodeGen/MachineCSE.cpp | 8 ++++----
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 7 ++++---
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/llvm/lib/CodeGen/MachineCSE.cpp b/llvm/lib/CodeGen/MachineCSE.cpp
index 1ebf7b96a3b94..6d14509c5934f 100644
--- a/llvm/lib/CodeGen/MachineCSE.cpp
+++ b/llvm/lib/CodeGen/MachineCSE.cpp
@@ -278,10 +278,10 @@ static bool isCallerPreservedOrConstPhysReg(MCRegister Reg,
(MRI.reservedRegsFrozen() && MRI.isConstantPhysReg(Reg));
}
-/// hasLivePhysRegDefUses - Return true if the specified instruction
-/// reads/writes physical registers (except for dead defs of physical
-/// registers). It also returns the physical register def by reference if it's
-/// the only one and the instruction does not use a physical register.
+/// hasLivePhysRegDefUses - Return true if the specified instruction read/write
+/// physical registers (except for dead defs of physical registers). It also
+/// returns the physical register def by reference if it's the only one and the
+/// instruction does not uses a physical register.
bool MachineCSEImpl::hasLivePhysRegDefUses(const MachineInstr *MI,
const MachineBasicBlock *MBB,
SmallSet<MCRegister, 8> &PhysRefs,
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 4990c9a74f6ee..4eec4a7cb2148 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -151,11 +151,12 @@ static bool resultDependsOnExec(const MachineInstr &MI) {
// Ignore comparisons which are only used masked with exec.
// This allows some hoisting/sinking of VALU comparisons.
if (MI.isCompare()) {
- if (SIInstrInfo::isVOPC(MI))
+ const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+ const MachineOperand &Dst = MI.getOperand(0);
+ if (!Dst.isReg() || !Dst.isDef() || MI.getNumExplicitDefs() > 1)
return false;
- const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
- Register DstReg = MI.getOperand(0).getReg();
+ Register DstReg = Dst.getReg();
if (!DstReg.isVirtual())
return true;
for (MachineInstr &Use : MRI.use_nodbg_instructions(DstReg)) {
>From cca5aff4fa88a64d572ed7153de511baa515d003 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <fharwath at amd.com>
Date: Mon, 7 Apr 2025 12:47:41 -0400
Subject: [PATCH 3/6] Change behavior on missing vdst
---
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 11 ++++++-----
llvm/lib/Target/AMDGPU/SIInstrInfo.h | 2 ++
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 4eec4a7cb2148..e8f85013cb60b 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -147,16 +147,17 @@ bool SIInstrInfo::isReallyTriviallyReMaterializable(
}
// Returns true if the scalar result of a VALU instruction depends on exec.
-static bool resultDependsOnExec(const MachineInstr &MI) {
+bool SIInstrInfo::resultDependsOnExec(const MachineInstr &MI) const {
// Ignore comparisons which are only used masked with exec.
// This allows some hoisting/sinking of VALU comparisons.
if (MI.isCompare()) {
const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
- const MachineOperand &Dst = MI.getOperand(0);
- if (!Dst.isReg() || !Dst.isDef() || MI.getNumExplicitDefs() > 1)
- return false;
+ const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::sdst);
+
+ if(!Dst)
+ return true;
- Register DstReg = Dst.getReg();
+ Register DstReg = Dst->getReg();
if (!DstReg.isVirtual())
return true;
for (MachineInstr &Use : MRI.use_nodbg_instructions(DstReg)) {
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.h b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
index d63225c067c9d..3ba48c784b96b 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
@@ -183,6 +183,8 @@ class SIInstrInfo final : public AMDGPUGenInstrInfo {
bool verifyCopy(const MachineInstr &MI, const MachineRegisterInfo &MRI,
StringRef &ErrInfo) const;
+ bool resultDependsOnExec(const MachineInstr &MI) const;
+
protected:
/// If the specific machine instruction is a instruction that moves/copies
/// value from one register to another register return destination and source
>From afa8d27fb9e8081650882513caa5209c64bd2dcd Mon Sep 17 00:00:00 2001
From: Frederik Harwath <fharwath at amd.com>
Date: Mon, 7 Apr 2025 13:05:05 -0400
Subject: [PATCH 4/6] clang-format changes
---
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index e8f85013cb60b..d4ff7728bea62 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -154,7 +154,7 @@ bool SIInstrInfo::resultDependsOnExec(const MachineInstr &MI) const {
const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::sdst);
- if(!Dst)
+ if (!Dst)
return true;
Register DstReg = Dst->getReg();
>From 75e56199117bd1a3b6e82ceecad2354f12e0d314 Mon Sep 17 00:00:00 2001
From: Frederik Harwath <frederik at harwath.name>
Date: Tue, 8 Apr 2025 12:25:50 +0200
Subject: [PATCH 5/6] Update llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
Co-authored-by: Matt Arsenault <arsenm2 at gmail.com>
---
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index d4ff7728bea62..66092e2a3e7ea 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -153,7 +153,6 @@ bool SIInstrInfo::resultDependsOnExec(const MachineInstr &MI) const {
if (MI.isCompare()) {
const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
const MachineOperand *Dst = getNamedOperand(MI, AMDGPU::OpName::sdst);
-
if (!Dst)
return true;
>From fe8a37ede026d18baa1c92dc22b41fc062f8934d Mon Sep 17 00:00:00 2001
From: Frederik Harwath <fharwath at amd.com>
Date: Tue, 8 Apr 2025 07:56:51 -0400
Subject: [PATCH 6/6] Rename test
---
...tr-info-vopc-no-exec.mir => si-instr-info-vopc-exec.mir} | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
rename llvm/test/CodeGen/AMDGPU/{si-instr-info-vopc-no-exec.mir => si-instr-info-vopc-exec.mir} (87%)
diff --git a/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir b/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-exec.mir
similarity index 87%
rename from llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir
rename to llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-exec.mir
index f1e9447dbd349..52a11ee3cdedf 100644
--- a/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-no-exec.mir
+++ b/llvm/test/CodeGen/AMDGPU/si-instr-info-vopc-exec.mir
@@ -1,12 +1,14 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
# RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -run-pass=machine-cse %s -o - | FileCheck %s
+# Check only that V_CMP_EQ_U32_e32 does not lead to a crash.
+
---
-name: vopc_comparisons_do_not_depend_on_exec
+name: depends_on_exec_check_can_handle_vopc
tracksRegLiveness: true
body: |
bb.0:
- ; CHECK-LABEL: name: vopc_comparisons_do_not_depend_on_exec
+ ; CHECK-LABEL: name: depends_on_exec_check_can_handle_vopc
; CHECK: [[DEF:%[0-9]+]]:vgpr_32 = IMPLICIT_DEF
; CHECK-NEXT: V_CMP_EQ_U32_e32 1, killed undef [[DEF]], implicit-def $vcc_lo, implicit $exec
; CHECK-NEXT: [[V_CNDMASK_B32_e32_:%[0-9]+]]:vgpr_32 = V_CNDMASK_B32_e32 16256, undef [[DEF]], implicit $vcc_lo, implicit $exec
More information about the llvm-commits
mailing list