[llvm] [SPIR-V][Codegen] Add isPhi bit to MCInstrDesc/MIR definitions (PR #110019)
Nathan Gauër via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 25 10:58:31 PDT 2024
https://github.com/Keenuts created https://github.com/llvm/llvm-project/pull/110019
the isPHI() function relied on the MIR opcode. This was fine as AFAIK no real target has a real PHI instruction. With SPIR-V, this assumption breaks.
The MachineVerifier has a special handling for PHI instructions to check liveness, but since this relied on the PHI/G_PHI opcode check, it was raising an error when the OpPhi MIR was checked.
Since the SPIR-V opcode is specific to the backend, I don't think checking the actual opcode in the MachineVerifier code is correct, so I added a bit in the instruction description, and applied it to the 3 existing PHI instruction I found (G_PHI, PHI, OpPhi).
Another different bit is the index of the first BB/reg pair: %res = PHI [reg, BB]
%res = OpPhi %reg [reg, BB]
The solution I had is to have a function in the MachineInstr returning the start index, allowing the MachineVerifier to work on both formats. Its slightly better, it works, but it's not THAT great. An alternative could be to add the index in the MCInstrDesc, this way, the index bit definition would be in the TD files, closer to the definition.
This patch reduces the amount of failling tests with EXPENSIVE_CHECKS from 120 to 113 (All fixed are in the SPIR-V backend).
Fixes #108844
>From 7334a2861bfc3a3b636a81000bb4f33daf221cf1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= <brioche at google.com>
Date: Wed, 25 Sep 2024 18:33:44 +0200
Subject: [PATCH] [SPIR-V][Codegen] Add isPhi bit to MIR
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
the isPHI() function relied on the MIR opcode. This was fine as AFAIK
no real target has a real PHI instruction. With SPIR-V, this assumption
breaks.
The MachineVerifier has a special handling for PHI instructions to check
liveness, but since this relied on the PHI/G_PHI opcode check, it was
raising an error when the OpPhi MIR was checked.
Since the SPIR-V opcode is specific to the backend, I don't think
checking the actual opcode in the MachineVerifier code is correct, so
I added a bit in the instruction description, and applied it to the 3
existing PHI instruction I found (G_PHI, PHI, OpPhi).
Another different bit is the index of the first BB/reg pair:
%res = PHI [reg, BB]
%res = OpPhi %reg [reg, BB]
The solution I had is to have a function in the MachineInstr returning
the start index, allowing the MachineVerifier to work on both formats.
Its slightly better, it works, but it's not THAT great.
An alternative could be to add the index in the MCInstrDesc, this way,
the index bit definition would be in the TD files, closer to the
definition.
This patch reduces the amount of failling tests with EXPENSIVE_CHECKS
from 120 to 113 (All fixed are in the SPIR-V backend).
Fixes #108844
Signed-off-by: Nathan Gauër <brioche at google.com>
---
llvm/include/llvm/CodeGen/MachineInstr.h | 15 ++++++++++++---
llvm/include/llvm/MC/MCInstrDesc.h | 5 +++++
llvm/include/llvm/Target/GenericOpcodes.td | 1 +
llvm/include/llvm/Target/Target.td | 2 ++
llvm/lib/CodeGen/MachineVerifier.cpp | 6 ++++--
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td | 6 ++++--
llvm/utils/TableGen/Common/CodeGenInstruction.cpp | 1 +
llvm/utils/TableGen/Common/CodeGenInstruction.h | 1 +
llvm/utils/TableGen/InstrInfoEmitter.cpp | 2 ++
9 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index 76a7b8662bae66..14ad5531f22c87 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1394,10 +1394,19 @@ class MachineInstr
return getOpcode() == TargetOpcode::JUMP_TABLE_DEBUG_INFO;
}
- bool isPHI() const {
- return getOpcode() == TargetOpcode::PHI ||
- getOpcode() == TargetOpcode::G_PHI;
+ bool isPHI() const { return getDesc().isPhi(); }
+
+ unsigned getIndexFirstPHIPair() const {
+ assert(isPHI());
+
+ if (getOpcode() == TargetOpcode::G_PHI || getOpcode() == TargetOpcode::PHI)
+ return 1;
+ // The only other instruction marked as PHI node is OpPhi, in the SPIR-V
+ // backend. The only difference is the [reg, BB] pairs starts at index 2,
+ // not 1.
+ return 2;
}
+
bool isKill() const { return getOpcode() == TargetOpcode::KILL; }
bool isImplicitDef() const { return getOpcode()==TargetOpcode::IMPLICIT_DEF; }
bool isInlineAsm() const {
diff --git a/llvm/include/llvm/MC/MCInstrDesc.h b/llvm/include/llvm/MC/MCInstrDesc.h
index ef0b3c0a73992b..3e7786bf2a3d8e 100644
--- a/llvm/include/llvm/MC/MCInstrDesc.h
+++ b/llvm/include/llvm/MC/MCInstrDesc.h
@@ -187,6 +187,7 @@ enum Flag {
Trap,
VariadicOpsAreDefs,
Authenticated,
+ Phi,
};
} // namespace MCID
@@ -292,6 +293,10 @@ class MCInstrDesc {
/// unconditional branches and return instructions.
bool isBarrier() const { return Flags & (1ULL << MCID::Barrier); }
+ /// Returns true if this instruction acts as a PHI node.
+ /// Not all PHI operands need to dominates the definition.
+ bool isPhi() const { return Flags & (1ULL << MCID::Phi); }
+
/// Returns true if this instruction part of the terminator for
/// a basic block. Typically this is things like return and branch
/// instructions.
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index f5e62dda6fd043..b8a4b9bba3cf67 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -96,6 +96,7 @@ def G_PHI : GenericInstruction {
let OutOperandList = (outs type0:$dst);
let InOperandList = (ins variable_ops);
let hasSideEffects = false;
+ let isPhi = true;
}
def G_FRAME_INDEX : GenericInstruction {
diff --git a/llvm/include/llvm/Target/Target.td b/llvm/include/llvm/Target/Target.td
index 3e037affe1cfd2..2b459f0df4228b 100644
--- a/llvm/include/llvm/Target/Target.td
+++ b/llvm/include/llvm/Target/Target.td
@@ -635,6 +635,7 @@ class Instruction : InstructionEncoding {
bit isBitcast = false; // Is this instruction a bitcast instruction?
bit isSelect = false; // Is this instruction a select instruction?
bit isBarrier = false; // Can control flow fall through this instruction?
+ bit isPhi = false; // Is this instruction a phi instruction?
bit isCall = false; // Is this instruction a call instruction?
bit isAdd = false; // Is this instruction an add instruction?
bit isTrap = false; // Is this instruction a trap instruction?
@@ -1174,6 +1175,7 @@ def PHI : StandardPseudoInstruction {
let InOperandList = (ins variable_ops);
let AsmString = "PHINODE";
let hasSideEffects = false;
+ let isPhi = true;
}
def INLINEASM : StandardPseudoInstruction {
let OutOperandList = (outs);
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 24a0f41775cc1d..ed843e7a7550f7 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -3213,7 +3213,8 @@ void MachineVerifier::calcRegsRequired() {
// Handle the PHI node.
for (const MachineInstr &MI : MBB.phis()) {
- for (unsigned i = 1, e = MI.getNumOperands(); i != e; i += 2) {
+ for (unsigned i = MI.getIndexFirstPHIPair(), e = MI.getNumOperands();
+ i != e; i += 2) {
// Skip those Operands which are undef regs or not regs.
if (!MI.getOperand(i).isReg() || !MI.getOperand(i).readsReg())
continue;
@@ -3268,7 +3269,8 @@ void MachineVerifier::checkPHIOps(const MachineBasicBlock &MBB) {
if (!DefReg.isVirtual())
report("Expected first PHI operand to be a virtual register", &MODef, 0);
- for (unsigned I = 1, E = Phi.getNumOperands(); I != E; I += 2) {
+ for (unsigned I = Phi.getIndexFirstPHIPair(), E = Phi.getNumOperands();
+ I != E; I += 2) {
const MachineOperand &MO0 = Phi.getOperand(I);
if (!MO0.isReg()) {
report("Expected PHI operand to be a register", &MO0, I);
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
index 51bacb00b1c515..6af0c857b1c582 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
@@ -615,8 +615,10 @@ def OpFwidthCoarse: UnOp<"OpFwidthCoarse", 215>;
// 3.42.17 Control-Flow Instructions
-def OpPhi: Op<245, (outs ID:$res), (ins TYPE:$type, ID:$var0, ID:$block0, variable_ops),
- "$res = OpPhi $type $var0 $block0">;
+let isPhi = 1 in {
+ def OpPhi: Op<245, (outs ID:$res), (ins TYPE:$type, ID:$var0, ID:$block0, variable_ops),
+ "$res = OpPhi $type $var0 $block0">;
+}
def OpLoopMerge: Op<246, (outs), (ins unknown:$merge, unknown:$continue, LoopControl:$lc, variable_ops),
"OpLoopMerge $merge $continue $lc">;
def OpSelectionMerge: Op<247, (outs), (ins unknown:$merge, SelectionControl:$sc),
diff --git a/llvm/utils/TableGen/Common/CodeGenInstruction.cpp b/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
index 452b084aa6f7d5..a8ae77bcca3efb 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenInstruction.cpp
@@ -469,6 +469,7 @@ CodeGenInstruction::CodeGenInstruction(const Record *R)
FastISelShouldIgnore = R->getValueAsBit("FastISelShouldIgnore");
variadicOpsAreDefs = R->getValueAsBit("variadicOpsAreDefs");
isAuthenticated = R->getValueAsBit("isAuthenticated");
+ isPhi = R->getValueAsBit("isPhi");
bool Unset;
mayLoad = R->getValueAsBitOrUnset("mayLoad", Unset);
diff --git a/llvm/utils/TableGen/Common/CodeGenInstruction.h b/llvm/utils/TableGen/Common/CodeGenInstruction.h
index 18294b157fedb1..c6374ba3366ff7 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstruction.h
+++ b/llvm/utils/TableGen/Common/CodeGenInstruction.h
@@ -286,6 +286,7 @@ class CodeGenInstruction {
bool hasChain_Inferred : 1;
bool variadicOpsAreDefs : 1;
bool isAuthenticated : 1;
+ bool isPhi : 1;
std::string DeprecatedReason;
bool HasComplexDeprecationPredicate;
diff --git a/llvm/utils/TableGen/InstrInfoEmitter.cpp b/llvm/utils/TableGen/InstrInfoEmitter.cpp
index 46605095ba85f8..3bd4d051692b04 100644
--- a/llvm/utils/TableGen/InstrInfoEmitter.cpp
+++ b/llvm/utils/TableGen/InstrInfoEmitter.cpp
@@ -1240,6 +1240,8 @@ void InstrInfoEmitter::emitRecord(
OS << "|(1ULL<<MCID::Select)";
if (Inst.isBarrier)
OS << "|(1ULL<<MCID::Barrier)";
+ if (Inst.isPhi)
+ OS << "|(1ULL<<MCID::Phi)";
if (Inst.hasDelaySlot)
OS << "|(1ULL<<MCID::DelaySlot)";
if (Inst.isCall)
More information about the llvm-commits
mailing list