[MachineVerifier] Query TargetInstrInfo for PHI nodes. #110507
Conversation
|
@llvm/pr-subscribers-backend-spir-v Author: Nathan Gauër (Keenuts) ChangesMachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an Alternative designs1st alternative:Add a bit in MCInstDesc for phi instructions (See #110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative:Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Fixes #108844 Full diff: https://github.com/llvm/llvm-project/pull/110507.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 07b59b241d9f9a..b4bf8ca984c9f4 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2278,6 +2278,32 @@ class TargetInstrInfo : public MCInstrInfo {
llvm_unreachable("unknown number of operands necessary");
}
+ // This this instruction a PHI node.
+ // This function should be favored over MI.isPHI() as it allows backends to
+ // define additional PHI instructions.
+ virtual bool isPhiInstr(const MachineInstr &MI) const {
+ return MI.getOpcode() == TargetOpcode::G_PHI ||
+ MI.getOpcode() == TargetOpcode::PHI;
+ }
+
+ // Returns the number of [Value, Src] pairs this phi instruction has.
+ // Only valid to call on a phi node.
+ virtual unsigned getNumPhiIncomingPair(const MachineInstr &MI) const {
+ assert(isPhiInstr(MI));
+ // G_PHI/PHI only have a single operand before the pairs. 2 Operands per
+ // pair.
+ return (MI.getNumOperands() - 1) / 2;
+ }
+
+ // Returns the |index|'th [Value, Src] pair of this phi instruction.
+ virtual std::pair<MachineOperand, MachineBasicBlock *>
+ getPhiIncomingPair(const MachineInstr &MI, unsigned index) const {
+ // Behavior for G_PHI/PHI instructions.
+ MachineOperand Operand = MI.getOperand(index * 2 + 1);
+ MachineBasicBlock *MBB = MI.getOperand(index * 2 + 2).getMBB();
+ return {Operand, MBB};
+ }
+
private:
mutable std::unique_ptr<MIRFormatter> Formatter;
unsigned CallFrameSetupOpcode, CallFrameDestroyOpcode;
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 24a0f41775cc1d..eb9d67fe9d14cf 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -2202,7 +2202,7 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
if (MI->getFlag(MachineInstr::NoConvergent) && !MCID.isConvergent())
report("NoConvergent flag expected only on convergent instructions.", MI);
- if (MI->isPHI()) {
+ if (TII->isPhiInstr(*MI)) {
if (MF->getProperties().hasProperty(
MachineFunctionProperties::Property::NoPHIs))
report("Found PHI instruction with NoPHIs property set", MI);
@@ -2705,7 +2705,7 @@ MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) {
break;
case MachineOperand::MO_MachineBasicBlock:
- if (MI->isPHI() && !MO->getMBB()->isSuccessor(MI->getParent()))
+ if (TII->isPhiInstr(*MI) && !MO->getMBB()->isSuccessor(MI->getParent()))
report("PHI operand is not in the CFG", MO, MONum);
break;
@@ -2776,7 +2776,7 @@ void MachineVerifier::checkLivenessAtUse(const MachineOperand *MO,
}
LiveQueryResult LRQ = LR.Query(UseIdx);
- bool HasValue = LRQ.valueIn() || (MI->isPHI() && LRQ.valueOut());
+ bool HasValue = LRQ.valueIn() || (TII->isPhiInstr(*MI) && LRQ.valueOut());
// Check if we have a segment at the use, note however that we only need one
// live subregister range, the others may be dead.
if (!HasValue && LaneMask.none()) {
@@ -2895,7 +2895,7 @@ void MachineVerifier::checkLiveness(const MachineOperand *MO, unsigned MONum) {
// Check LiveInts liveness and kill.
if (LiveInts && !LiveInts->isNotInMIMap(*MI)) {
SlotIndex UseIdx;
- if (MI->isPHI()) {
+ if (TII->isPhiInstr(*MI)) {
// PHI use occurs on the edge, so check for live out here instead.
UseIdx = LiveInts->getMBBEndIdx(
MI->getOperand(MONum + 1).getMBB()).getPrevSlot();
@@ -2926,7 +2926,7 @@ void MachineVerifier::checkLiveness(const MachineOperand *MO, unsigned MONum) {
continue;
checkLivenessAtUse(MO, MONum, UseIdx, SR, Reg, SR.LaneMask);
LiveQueryResult LRQ = SR.Query(UseIdx);
- if (LRQ.valueIn() || (MI->isPHI() && LRQ.valueOut()))
+ if (LRQ.valueIn() || (TII->isPhiInstr(*MI) && LRQ.valueOut()))
LiveInMask |= SR.LaneMask;
}
// At least parts of the register has to be live at the use.
@@ -2936,7 +2936,7 @@ void MachineVerifier::checkLiveness(const MachineOperand *MO, unsigned MONum) {
report_context(UseIdx);
}
// For PHIs all lanes should be live
- if (MI->isPHI() && LiveInMask != MOMask) {
+ if (TII->isPhiInstr(*MI) && LiveInMask != MOMask) {
report("Not all lanes of PHI source live at use", MO, MONum);
report_context(*LI);
report_context(UseIdx);
@@ -2987,7 +2987,7 @@ void MachineVerifier::checkLiveness(const MachineOperand *MO, unsigned MONum) {
// must be live in. PHI instructions are handled separately.
if (MInfo.regsKilled.count(Reg))
report("Using a killed virtual register", MO, MONum);
- else if (!MI->isPHI())
+ else if (!TII->isPhiInstr(*MI))
MInfo.vregsLiveIn.insert(std::make_pair(Reg, MI));
}
}
@@ -3211,17 +3211,22 @@ void MachineVerifier::calcRegsRequired() {
todo.insert(Pred);
}
- // Handle the PHI node.
- for (const MachineInstr &MI : MBB.phis()) {
- for (unsigned i = 1, e = MI.getNumOperands(); i != e; i += 2) {
+ // Handle the PHI nodes.
+ // Note: MBB.phis() only returns range containing PHI/G_PHI instructions.
+ // MachineVerifier checks MIR post-ISel, and some backends do have their own
+ // phi nodes.
+ for (const MachineInstr &MI : MBB) {
+ // PHI nodes must be the first instructions of the MBB.
+ if (!TII->isPhiInstr(MI))
+ break;
+ for (unsigned i = 0; i < TII->getNumPhiIncomingPair(MI); ++i) {
+ auto [Op, Pred] = TII->getPhiIncomingPair(MI, i);
// Skip those Operands which are undef regs or not regs.
- if (!MI.getOperand(i).isReg() || !MI.getOperand(i).readsReg())
+ if (!Op.isReg() || !Op.readsReg())
continue;
// Get register and predecessor for one PHI edge.
- Register Reg = MI.getOperand(i).getReg();
- const MachineBasicBlock *Pred = MI.getOperand(i + 1).getMBB();
-
+ Register Reg = Op.getReg();
BBInfo &PInfo = MBBInfoMap[Pred];
if (PInfo.addRequired(Reg))
todo.insert(Pred);
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
index e8a6b4fdbae977..0029bcb830e381 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp
@@ -267,3 +267,27 @@ bool SPIRVInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
}
return false;
}
+
+// The SPIR-V backends can emit the OpPhi instruction.
+bool SPIRVInstrInfo::isPhiInstr(const MachineInstr &MI) const {
+ return TargetInstrInfo::isPhiInstr(MI) || MI.getOpcode() == SPIRV::OpPhi;
+}
+
+unsigned SPIRVInstrInfo::getNumPhiIncomingPair(const MachineInstr &MI) const {
+ // OpPhi has 2 operands before the [Value, Src] pairs.
+ if (MI.getOpcode() == SPIRV::OpPhi)
+ return (MI.getNumOperands() - 2) / 2;
+ return TargetInstrInfo::getNumPhiIncomingPair(MI);
+}
+
+std::pair<MachineOperand, MachineBasicBlock *>
+SPIRVInstrInfo::getPhiIncomingPair(const MachineInstr &MI,
+ unsigned index) const {
+ if (MI.getOpcode() != SPIRV::OpPhi)
+ return TargetInstrInfo::getPhiIncomingPair(MI, index);
+
+ // Skip the first 2 operands (dst, type), and access each [Value, Src] pairs.
+ MachineOperand Operand = MI.getOperand(index * 2 + 2);
+ MachineBasicBlock *MBB = MI.getOperand(index * 2 + 3).getMBB();
+ return {Operand, MBB};
+}
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
index 67d2d979cb5a15..bae008adaa85c9 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
@@ -54,6 +54,11 @@ class SPIRVInstrInfo : public SPIRVGenInstrInfo {
bool KillSrc, bool RenamableDest = false,
bool RenamableSrc = false) const override;
bool expandPostRAPseudo(MachineInstr &MI) const override;
+
+ bool isPhiInstr(const MachineInstr &MI) const override;
+ unsigned getNumPhiIncomingPair(const MachineInstr &MI) const override;
+ std::pair<MachineOperand, MachineBasicBlock *>
+ getPhiIncomingPair(const MachineInstr &MI, unsigned index) const override;
};
namespace SPIRV {
|
| // This this instruction a PHI node. | ||
| // This function should be favored over MI.isPHI() as it allows backends to | ||
| // define additional PHI instructions. | ||
| virtual bool isPhiInstr(const MachineInstr &MI) const { | ||
| return MI.getOpcode() == TargetOpcode::G_PHI || | ||
| MI.getOpcode() == TargetOpcode::PHI; | ||
| } | ||
|
|
||
| // Returns the number of [Value, Src] pairs this phi instruction has. | ||
| // Only valid to call on a phi node. | ||
| virtual unsigned getNumPhiIncomingPair(const MachineInstr &MI) const { | ||
| assert(isPhiInstr(MI)); | ||
| // G_PHI/PHI only have a single operand before the pairs. 2 Operands per | ||
| // pair. | ||
| return (MI.getNumOperands() - 1) / 2; | ||
| } | ||
|
|
||
| // Returns the |index|'th [Value, Src] pair of this phi instruction. | ||
| virtual std::pair<MachineOperand, MachineBasicBlock *> | ||
| getPhiIncomingPair(const MachineInstr &MI, unsigned index) const { | ||
| // Behavior for G_PHI/PHI instructions. | ||
| MachineOperand Operand = MI.getOperand(index * 2 + 1); | ||
| MachineBasicBlock *MBB = MI.getOperand(index * 2 + 2).getMBB(); | ||
| return {Operand, MBB}; | ||
| } |
There was a problem hiding this comment.
I don't think the special case SPIRV operator should be treated like a generic phi like this (at least, not in any actual codegen uses). Can you attach sample failing MIR cases to the issue (or better yet, add machine verifier tests to this PR)? I think we'd be better off only touching something visible to the verifier, or otherwise avoiding clearing the NoPhis property for spirv
There was a problem hiding this comment.
Sure.
A failing test which now passes is llvm/test/CodeGen/SPIRV/branching/if-merging.ll.
Added -verify-machineinstrs to fixed tests to the current PR (adding this flag on main break those)
bb.1.entry:
successors: %bb.2, %bb.3
[...]
OpBranchConditional %5:iid, %bb.2, %bb.3
bb.2.true_label:
; predecessors: %bb.1
successors: %bb.4(0x80000000); %bb.4(100.00%)
%12:iid = OpFunctionCall %2:type, @foo
[...]
OpBranch %bb.4
bb.3.false_label:
; predecessors: %bb.1
successors: %bb.4(0x80000000); %bb.4(100.00%)
%8:iid = OpFunctionCall %2:type, @bar
[...]
OpBranch %bb.4
bb.4.merge_label:
; predecessors: %bb.3, %bb.2
%15:id = OpPhi %2:type, %12:iid, %bb.2, %8:iid, %bb.3
OpName %15:id, 118
OpReturnValue %15:id
# End machine code for function test_if.
*** Bad machine code: Virtual register defs don't dominate all uses. ***
- function: test_if
- v. register: %8
*** Bad machine code: Virtual register defs don't dominate all uses. ***
- function: test_if
- v. register: %12
Is the OpPhi was handled as a PHI, it would pass MachineVerifer tests, as %12 is defined if coming from bb.2, and same for %8/bb.3.
There was a problem hiding this comment.
I meant tests that hit the error in test/MachineVerifier, not tests that you are trying to make not fail the verifier
There was a problem hiding this comment.
I don't think the special case SPIRV operator should be treated like a generic phi like this (at least, not in any actual codegen uses).
Why that? It's also a PHI node, exact same behavior & requirements (beginning of a BB, 1 pair by predecessor, only labels as predecessor argument, etc)
There was a problem hiding this comment.
I meant tests that hit the error in test/MachineVerifier, not tests that you are trying to make not fail the verifier
Added a new MachineVerifer test: llvm/test/MachineVerifier/verifier-phi-spirv.mir
There was a problem hiding this comment.
exact same behavior & requirements
It's not the same. PHI requires an entry for each incoming edge, and OpPhi requires one entry per predecessor (this is another case where it will likely be easier to conform the SPIRV operation to the generic's form, and then just fix up the encoding f
There was a problem hiding this comment.
I'm still inclined to think it will be easier to just select to PHI and only treat the conversion to OpPhi as a late final encoding detail.
The ordinary compilation flow emits the PHI, which is later destructured to copies at the start of register allocation. The SPIRV analog would be to adjust at a similarly late point to something closer to OpPhi
The point of the backend is to emit SPIRV at the end, that doesn't require you to turn MIR into SPIRV. It's a different representation
| @@ -0,0 +1,31 @@ | |||
| # RUN: llc -o - %s -mtriple=spirv-unknown-vulkan-compute -verify-machineinstrs -run-pass=none | FileCheck %s | |||
There was a problem hiding this comment.
Don't need -verify-machineinstrs, the parser always verifies
| // This this instruction a PHI node. | ||
| // This function should be favored over MI.isPHI() as it allows backends to | ||
| // define additional PHI instructions. | ||
| virtual bool isPhiInstr(const MachineInstr &MI) const { | ||
| return MI.getOpcode() == TargetOpcode::G_PHI || | ||
| MI.getOpcode() == TargetOpcode::PHI; | ||
| } | ||
|
|
||
| // Returns the number of [Value, Src] pairs this phi instruction has. | ||
| // Only valid to call on a phi node. | ||
| virtual unsigned getNumPhiIncomingPair(const MachineInstr &MI) const { | ||
| assert(isPhiInstr(MI)); | ||
| // G_PHI/PHI only have a single operand before the pairs. 2 Operands per | ||
| // pair. | ||
| return (MI.getNumOperands() - 1) / 2; | ||
| } | ||
|
|
||
| // Returns the |index|'th [Value, Src] pair of this phi instruction. | ||
| virtual std::pair<MachineOperand, MachineBasicBlock *> | ||
| getPhiIncomingPair(const MachineInstr &MI, unsigned index) const { | ||
| // Behavior for G_PHI/PHI instructions. | ||
| MachineOperand Operand = MI.getOperand(index * 2 + 1); | ||
| MachineBasicBlock *MBB = MI.getOperand(index * 2 + 2).getMBB(); | ||
| return {Operand, MBB}; | ||
| } |
There was a problem hiding this comment.
exact same behavior & requirements
It's not the same. PHI requires an entry for each incoming edge, and OpPhi requires one entry per predecessor (this is another case where it will likely be easier to conform the SPIRV operation to the generic's form, and then just fix up the encoding f
|
We should not say that We may describe the issue so that GenCode doesn’t expect that after lowering a target still supports a higher-level concept/opcode representing a PHI node. SPIR-V has a PHI opcode, and it doesn’t match with what GenCode expects from a lowered code. I think that this PR presents a normal engineering practice of using defaults and overrides to resolve this issue. |
|
In fact #108844 might have been worded as follows:
In other words, it doesn't matter what is the difference between phi and OpPhi when we discuss this issue. Machine Verifier may not consider this a bad code, because Instruction Selection results in target-specific instructions, and by the SPIR-V spec (target-specific) OpPhi implements the φ node. |
There was a problem hiding this comment.
After the discussions at the LLVM Developers' Meeting and general vision other contributors have about GlobalISel, I think this still might be the best way forward. Workarounds in the code gen pipeline significantly contribute both to logical complexity and compilation speed. Also, from a purist point of view, selected instructions should not require any further "selection". Using TII is a reasonable flexibility that in this and other cases may help other targets in the future. Target specific information is used in other cases in MachineVerifier.
I am open to discuss this (e.g. at our next SPIR-V backend meeting on Wednesday next week), but overall the change is justified and rather helps.
Keenuts
force-pushed
the
fix-108844-isphi
branch
from
3fc31a5 to
5e229bc
Compare
|
Thanks for the reviews! |
Keenuts
force-pushed
the
fix-108844-isphi
branch
2 times, most recently
from
f59cbd7 to
5f10a51
Compare
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an `isPhiInstr`. Alternative designs =================== 1st alternative: ---------------- Add a bit in MCInstDesc for phi instructions (See llvm#110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative: ---------------- Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using `isPHI` to also pass TII. This is a very invasive change, which impacts almost every backends, and several passes. Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
Signed-off-by: Nathan Gauër <[email protected]>
Signed-off-by: Nathan Gauër <[email protected]>
Signed-off-by: Nathan Gauër <[email protected]>
Signed-off-by: Nathan Gauër <[email protected]>
Keenuts
force-pushed
the
fix-108844-isphi
branch
from
5f10a51 to
e45cc4e
Compare
…9202) This PR improves general validity of emitted code between passes due to generation of `TargetOpcode::PHI` instead of `SPIRV::OpPhi` after Instruction Selection, fixing generation of OpTypePointer instructions and using of proper virtual register classes. Using `TargetOpcode::PHI` instead of `SPIRV::OpPhi` after Instruction Selection has a benefit to support existing optimization passes immediately, as an alternative path to disable those passes that use `MI.isPHI()`. This PR makes it possible thus to revert #116060 actions and get back to use the `MachineSink` pass. This PR is a solution of the problem discussed in details in #110507. It accepts an advice from code reviewers of the PR #110507 to postpone generation of OpPhi rather than to patch CodeGen. This solution allows to unblock improvements wrt. expensive checks and makes it unrelated to the general points of the discussion about OpPhi vs. G_PHI/PHI. This PR contains numerous small patches of emitted code validity that allows to substantially pass rate with expensive checks. Namely, the test suite with expensive checks set ON now has only 12 fails out of 569 total test cases. FYI @bogner
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs).
There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an
isPhiInstr.Alternative designs
1st alternative:
Add a bit in MCInstDesc for phi instructions (See #110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense.
Additionally, it doesn't help handling the changing operands layout, and still requires some additional info from the target to do so.
2nd alternative:
Refactorize
MachineInstr::isPHI()to useTargetInstrInfo. This was almost possible, asMachineInstroften has a parent MBB, and thus a parent MF which links to the TargetInstrInfo.In MachineInstr:
this->getMF().getTargetInstrInfo().isPhiInstr(*this)This however breaks as soon as the
MachineInstris removed from it's parent block.Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using
isPHIto also pass TII. This is a very invasive change, which impacts almost every backends, and several passes.Fixes #108844