From 67d7fe7f19bef5807b15e28b46b113f78ae40d45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 30 Sep 2024 13:55:28 +0200 Subject: [PATCH 1/6] [MachineVerifier] Query TargetInstrInfo for PHI nodes. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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 #108844 Signed-off-by: Nathan Gauër --- llvm/include/llvm/CodeGen/TargetInstrInfo.h | 26 ++++++++++++++++ llvm/lib/CodeGen/MachineVerifier.cpp | 33 ++++++++++++--------- llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp | 24 +++++++++++++++ llvm/lib/Target/SPIRV/SPIRVInstrInfo.h | 5 ++++ 4 files changed, 74 insertions(+), 14 deletions(-) diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h index 07b59b241d9f9..b4bf8ca984c9f 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 + 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 Formatter; unsigned CallFrameSetupOpcode, CallFrameDestroyOpcode; diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index e2c09fe25d55c..f2a6b4c9a454b 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -2231,7 +2231,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); @@ -2734,7 +2734,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; @@ -2805,7 +2805,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()) { @@ -2924,7 +2924,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(); @@ -2955,7 +2955,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. @@ -2965,7 +2965,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); @@ -3016,7 +3016,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)); } } @@ -3240,17 +3240,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 bd9e77e9427c0..fe84f4c0963c8 100644 --- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp @@ -269,3 +269,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 +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 67d2d979cb5a1..bae008adaa85c 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 + getPhiIncomingPair(const MachineInstr &MI, unsigned index) const override; }; namespace SPIRV { From e1c20ee14440d5a738eb1beb4277796ce7d1ce0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 30 Sep 2024 17:32:33 +0200 Subject: [PATCH 2/6] add -machine-verifier flags --- llvm/test/CodeGen/SPIRV/branching/if-merging.ll | 2 +- llvm/test/CodeGen/SPIRV/llvm-intrinsics/memset.ll | 2 +- .../CodeGen/SPIRV/optimizations/add-check-overflow.ll | 4 ++-- .../CodeGen/SPIRV/passes/translate-aggregate-uaddo.ll | 10 +++++----- llvm/test/CodeGen/SPIRV/structurizer/cf.cond-op.ll | 2 +- .../SPIRV/structurizer/cf.for.short-circuited-cond.ll | 2 +- .../structurizer/cf.while.short-circuited-cond.ll | 2 +- .../CodeGen/SPIRV/structurizer/condition-linear.ll | 2 +- llvm/test/CodeGen/SPIRV/transcoding/OpImageReadMS.ll | 2 +- 9 files changed, 14 insertions(+), 14 deletions(-) diff --git a/llvm/test/CodeGen/SPIRV/branching/if-merging.ll b/llvm/test/CodeGen/SPIRV/branching/if-merging.ll index 52eeb216234e5..3fb7e000c1025 100644 --- a/llvm/test/CodeGen/SPIRV/branching/if-merging.ll +++ b/llvm/test/CodeGen/SPIRV/branching/if-merging.ll @@ -1,4 +1,4 @@ -; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -verify-machineinstrs | FileCheck %s ;; NOTE: This does not check for structured control-flow operations. diff --git a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/memset.ll b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/memset.ll index e7a986980f250..40b046a9dd622 100644 --- a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/memset.ll +++ b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/memset.ll @@ -1,4 +1,4 @@ -; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -verify-machineinstrs | FileCheck %s ; CHECK-DAG: OpDecorate %[[#Memset_p0i32:]] LinkageAttributes "spirv.llvm_memset_p0_i32" Export ; CHECK-DAG: OpDecorate %[[#Memset_p3i32:]] LinkageAttributes "spirv.llvm_memset_p3_i32" Export diff --git a/llvm/test/CodeGen/SPIRV/optimizations/add-check-overflow.ll b/llvm/test/CodeGen/SPIRV/optimizations/add-check-overflow.ll index 1a630f77a44c5..dfda2d0920e91 100644 --- a/llvm/test/CodeGen/SPIRV/optimizations/add-check-overflow.ll +++ b/llvm/test/CodeGen/SPIRV/optimizations/add-check-overflow.ll @@ -2,10 +2,10 @@ ; in the special case when those intrinsics are being generated by the CodeGenPrepare; ; pass during translations with optimization (note -O3 in llc arguments). -; RUN: llc -O3 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s +; RUN: llc -O3 -mtriple=spirv32-unknown-unknown %s -o - -verify-machineinstrs | FileCheck %s ; RUN: %if spirv-tools %{ llc -O3 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %} -; RUN: llc -O3 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s +; RUN: llc -O3 -mtriple=spirv64-unknown-unknown %s -o - -verify-machineinstrs | FileCheck %s ; RUN: %if spirv-tools %{ llc -O3 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %} ; CHECK-DAG: OpName %[[Val:.*]] "math" diff --git a/llvm/test/CodeGen/SPIRV/passes/translate-aggregate-uaddo.ll b/llvm/test/CodeGen/SPIRV/passes/translate-aggregate-uaddo.ll index cd4d9325c7659..9030b00388032 100644 --- a/llvm/test/CodeGen/SPIRV/passes/translate-aggregate-uaddo.ll +++ b/llvm/test/CodeGen/SPIRV/passes/translate-aggregate-uaddo.ll @@ -1,11 +1,11 @@ ; This test shows how value attributes are being passed during different translation steps. ; See also test/CodeGen/SPIRV/optimizations/add-check-overflow.ll. -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=prepare-functions 2>&1 | FileCheck %s --check-prefix=CHECK-PREPARE +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=prepare-functions 2>&1 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-PREPARE ; Intrinsics with aggregate return type are not substituted/removed. ; CHECK-PREPARE: @llvm.uadd.with.overflow.i32 -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=emit-intrinsics 2>&1 | FileCheck %s --check-prefix=CHECK-IR +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=emit-intrinsics 2>&1 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-IR ; Aggregate data are wrapped into @llvm.fake.use(), ; and their attributes are packed into a metadata for @llvm.spv.value.md(). ; CHECK-IR: %[[R1:.*]] = call { i32, i1 } @llvm.uadd.with.overflow.i32 @@ -18,20 +18,20 @@ ; Origin data type of the value. ; CHECK-IR: !1 = !{{[{]}}{{[{]}} i32, i1 {{[}]}} poison{{[}]}} -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=irtranslator 2>&1 | FileCheck %s --check-prefix=CHECK-GMIR +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=irtranslator 2>&1 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-GMIR ; Required info succeeded to get through IRTranslator. ; CHECK-GMIR: %[[phires:.*]]:_(s32) = G_PHI ; CHECK-GMIR: %[[math:.*]]:id(s32), %[[ov:.*]]:_(s1) = G_UADDO %[[phires]]:_, %[[#]]:_ ; CHECK-GMIR: G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.spv.value.md), !0 ; CHECK-GMIR: FAKE_USE %[[math]]:id(s32), %[[ov]]:_(s1) -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=spirv-prelegalizer 2>&1 | FileCheck %s --check-prefix=CHECK-PRE +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=spirv-prelegalizer 2>&1 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-PRE ; Internal service instructions are consumed. ; CHECK-PRE: G_UADDO ; CHECK-PRE-NO: llvm.spv.value.md ; CHECK-PRE-NO: FAKE_USE -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=instruction-select 2>&1 | FileCheck %s --check-prefix=CHECK-ISEL +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -print-after=instruction-select 2>&1 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-ISEL ; Names and types are restored and correctly encoded. Correct instruction selection is completed. ; CHECK-ISEL-DAG: %[[int32:.*]]:type = OpTypeInt 32, 0 ; CHECK-ISEL-DAG: %[[struct:.*]]:type = OpTypeStruct %[[int32]]:type, %[[int32]]:type diff --git a/llvm/test/CodeGen/SPIRV/structurizer/cf.cond-op.ll b/llvm/test/CodeGen/SPIRV/structurizer/cf.cond-op.ll index 86033608deb6e..f7a5931e40174 100644 --- a/llvm/test/CodeGen/SPIRV/structurizer/cf.cond-op.ll +++ b/llvm/test/CodeGen/SPIRV/structurizer/cf.cond-op.ll @@ -1,4 +1,4 @@ -; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - | FileCheck %s +; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - -verify-machineinstrs | FileCheck %s ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan-compute %s -o - -filetype=obj | spirv-val %} target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1" diff --git a/llvm/test/CodeGen/SPIRV/structurizer/cf.for.short-circuited-cond.ll b/llvm/test/CodeGen/SPIRV/structurizer/cf.for.short-circuited-cond.ll index 07c20ebadd159..7f50f498e5b61 100644 --- a/llvm/test/CodeGen/SPIRV/structurizer/cf.for.short-circuited-cond.ll +++ b/llvm/test/CodeGen/SPIRV/structurizer/cf.for.short-circuited-cond.ll @@ -1,4 +1,4 @@ -; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - | FileCheck %s +; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - -verify-machineinstrs | FileCheck %s ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan-compute %s -o - -filetype=obj | spirv-val %} ; diff --git a/llvm/test/CodeGen/SPIRV/structurizer/cf.while.short-circuited-cond.ll b/llvm/test/CodeGen/SPIRV/structurizer/cf.while.short-circuited-cond.ll index 79ce0c15e5c43..f65176a4249a8 100644 --- a/llvm/test/CodeGen/SPIRV/structurizer/cf.while.short-circuited-cond.ll +++ b/llvm/test/CodeGen/SPIRV/structurizer/cf.while.short-circuited-cond.ll @@ -1,4 +1,4 @@ -; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - | FileCheck %s +; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - -verify-machineinstrs | FileCheck %s ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan-compute %s -o - -filetype=obj | spirv-val %} ; diff --git a/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll b/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll index 71f3ce9263da5..547a668b4d9c0 100644 --- a/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll +++ b/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll @@ -1,4 +1,4 @@ -; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - | FileCheck %s +; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - -verify-machineinstrs | FileCheck %s --match-full-lines ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan-compute %s -o - -filetype=obj | spirv-val %} target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1" diff --git a/llvm/test/CodeGen/SPIRV/transcoding/OpImageReadMS.ll b/llvm/test/CodeGen/SPIRV/transcoding/OpImageReadMS.ll index f4e34c325e601..ed57720c51616 100644 --- a/llvm/test/CodeGen/SPIRV/transcoding/OpImageReadMS.ll +++ b/llvm/test/CodeGen/SPIRV/transcoding/OpImageReadMS.ll @@ -1,4 +1,4 @@ -; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-SPIRV ; CHECK-SPIRV: %[[#]] = OpImageRead %[[#]] %[[#]] %[[#]] Sample %[[#]] From 1b8d48893707951bf481fe50a6531eb83a1969c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 30 Sep 2024 18:51:54 +0200 Subject: [PATCH 3/6] add MachineVerifier test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- .../MachineVerifier/verifier-phi-spirv.mir | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 llvm/test/MachineVerifier/verifier-phi-spirv.mir diff --git a/llvm/test/MachineVerifier/verifier-phi-spirv.mir b/llvm/test/MachineVerifier/verifier-phi-spirv.mir new file mode 100644 index 0000000000000..4c654d55ebf17 --- /dev/null +++ b/llvm/test/MachineVerifier/verifier-phi-spirv.mir @@ -0,0 +1,31 @@ +# RUN: llc -o - %s -mtriple=spirv-unknown-vulkan-compute -verify-machineinstrs -run-pass=none | FileCheck %s +# REQUIRES: spirv-registered-target + +# This should cleanly pass the machine verifier +--- +# CHECK-LABEL: name: func0 +# CHECK: %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2 +name: func0 +tracksRegLiveness: true +noPhis: false +body: | + bb.0: + %0:type = OpTypeBool + %1:type = OpTypeInt 32, 0 + %2:iid = OpFunctionParameter %1 + %3:iid = OpFunctionParameter %1 + %4:iid = OpIEqual %0, %2, %3 + OpBranchConditional %4, %bb.1, %bb.2 + + bb.1: + %5:iid = OpLoad %1, %2 + OpBranch %bb.3 + + bb.2: + %6:iid = OpLoad %1, %3 + OpBranch %bb.3 + + bb.3: + %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2 +... +--- From 4568a24b128656a5f54e8674d508154faae348e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Tue, 29 Oct 2024 17:45:52 +0100 Subject: [PATCH 4/6] pr feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- llvm/include/llvm/CodeGen/TargetInstrInfo.h | 6 ++-- llvm/lib/CodeGen/MachineVerifier.cpp | 4 +-- llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp | 2 +- llvm/test/MachineVerifier/SPIRV/lit.local.cfg | 2 ++ .../SPIRV/verifier-phi-duplicate-pred.mir | 34 +++++++++++++++++++ .../verifier-phi.mir} | 6 ++-- 6 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 llvm/test/MachineVerifier/SPIRV/lit.local.cfg create mode 100644 llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir rename llvm/test/MachineVerifier/{verifier-phi-spirv.mir => SPIRV/verifier-phi.mir} (70%) diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h index b4bf8ca984c9f..720c3255f5bac 100644 --- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h +++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h @@ -2278,9 +2278,9 @@ class TargetInstrInfo : public MCInstrInfo { llvm_unreachable("unknown number of operands necessary"); } - // This this instruction a PHI node. + // Returns true if this instruction is a PHI node. // This function should be favored over MI.isPHI() as it allows backends to - // define additional PHI instructions. + // report actual PHI instructions in their ISA as such. virtual bool isPhiInstr(const MachineInstr &MI) const { return MI.getOpcode() == TargetOpcode::G_PHI || MI.getOpcode() == TargetOpcode::PHI; @@ -2292,7 +2292,7 @@ class TargetInstrInfo : public MCInstrInfo { assert(isPhiInstr(MI)); // G_PHI/PHI only have a single operand before the pairs. 2 Operands per // pair. - return (MI.getNumOperands() - 1) / 2; + return (MI.getNumOperands() - 1) >> 1; } // Returns the |index|'th [Value, Src] pair of this phi instruction. diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp index f2a6b4c9a454b..3212d3b1b3dd1 100644 --- a/llvm/lib/CodeGen/MachineVerifier.cpp +++ b/llvm/lib/CodeGen/MachineVerifier.cpp @@ -3242,8 +3242,8 @@ void MachineVerifier::calcRegsRequired() { // 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. + // MachineVerifier checks MIR post-ISel, and some target ISA can have actual + // PHI instructions that would be missed in MBB.phis() (e.g. SPIR-V). for (const MachineInstr &MI : MBB) { // PHI nodes must be the first instructions of the MBB. if (!TII->isPhiInstr(MI)) diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp index fe84f4c0963c8..1d8f81f1e0d40 100644 --- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.cpp @@ -278,7 +278,7 @@ bool SPIRVInstrInfo::isPhiInstr(const MachineInstr &MI) const { 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 (MI.getNumOperands() - 2) >> 1; return TargetInstrInfo::getNumPhiIncomingPair(MI); } diff --git a/llvm/test/MachineVerifier/SPIRV/lit.local.cfg b/llvm/test/MachineVerifier/SPIRV/lit.local.cfg new file mode 100644 index 0000000000000..78dd74cd6dc63 --- /dev/null +++ b/llvm/test/MachineVerifier/SPIRV/lit.local.cfg @@ -0,0 +1,2 @@ +if not "SPIRV" in config.root.targets: + config.unsupported = True diff --git a/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir b/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir new file mode 100644 index 0000000000000..a944569f6b51e --- /dev/null +++ b/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir @@ -0,0 +1,34 @@ +# RUN: llc -o - %s -mtriple=spirv-unknown-vulkan-compute -run-pass=none | FileCheck %s +# REQUIRES: spirv-registered-target + +# This should cleanly pass the machine verifier. +--- +# CHECK-LABEL: name: func0 +# CHECK: %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2 +name: func0 +tracksRegLiveness: true +noPhis: false +body: | + bb.0: + %0:type = OpTypeBool + %1:type = OpTypeInt 32, 0 + %2:iid = OpFunctionParameter %1 + %3:iid = OpFunctionParameter %1 + %4:iid = OpIEqual %0, %2, %3 + OpBranchConditional %4, %bb.1, %bb.2 + + bb.1: + %5:iid = OpLoad %1, %2 + OpBranch %bb.3 + + bb.2: + %6:iid = OpLoad %1, %3 + OpBranch %bb.3 + + bb.3: +# This test validates a subtle SPIR-V difference with PHI nodes: +# It is valid to have the same predecessor present twice in the instruction, +# as long as the associated value is the same. + %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2, %5, %bb.1 +... +--- diff --git a/llvm/test/MachineVerifier/verifier-phi-spirv.mir b/llvm/test/MachineVerifier/SPIRV/verifier-phi.mir similarity index 70% rename from llvm/test/MachineVerifier/verifier-phi-spirv.mir rename to llvm/test/MachineVerifier/SPIRV/verifier-phi.mir index 4c654d55ebf17..bbd6cc1ca0e39 100644 --- a/llvm/test/MachineVerifier/verifier-phi-spirv.mir +++ b/llvm/test/MachineVerifier/SPIRV/verifier-phi.mir @@ -1,7 +1,9 @@ -# RUN: llc -o - %s -mtriple=spirv-unknown-vulkan-compute -verify-machineinstrs -run-pass=none | FileCheck %s +# RUN: llc -o - %s -mtriple=spirv-unknown-vulkan-compute -run-pass=none | FileCheck %s # REQUIRES: spirv-registered-target -# This should cleanly pass the machine verifier +# This should cleanly pass the machine verifier. +# Nothing specific regarding PHI requirements, just a slight change in +# the operand list vs G_PHI. --- # CHECK-LABEL: name: func0 # CHECK: %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2 From fbf29b2ffbf519aa484affdf2ec03640818b4115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Tue, 29 Oct 2024 18:08:04 +0100 Subject: [PATCH 5/6] move comment above in test file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- .../MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir b/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir index a944569f6b51e..8191838b7d512 100644 --- a/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir +++ b/llvm/test/MachineVerifier/SPIRV/verifier-phi-duplicate-pred.mir @@ -2,6 +2,9 @@ # REQUIRES: spirv-registered-target # This should cleanly pass the machine verifier. +# This test validates a subtle SPIR-V difference with PHI nodes: +# It is valid to have the same predecessor present twice in the instruction, +# as long as the associated value is the same. --- # CHECK-LABEL: name: func0 # CHECK: %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2 @@ -26,9 +29,6 @@ body: | OpBranch %bb.3 bb.3: -# This test validates a subtle SPIR-V difference with PHI nodes: -# It is valid to have the same predecessor present twice in the instruction, -# as long as the associated value is the same. %7:id = OpPhi %1, %5, %bb.1, %6, %bb.2, %5, %bb.1 ... --- From e45cc4eebcf997b02d66d76fa4db44b21306b31c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 4 Nov 2024 14:03:06 +0100 Subject: [PATCH 6/6] rebased, fix broken test (r2m now executed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- .../SPIRV/structurizer/condition-linear.ll | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll b/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll index 547a668b4d9c0..6c90585d6a935 100644 --- a/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll +++ b/llvm/test/CodeGen/SPIRV/structurizer/condition-linear.ll @@ -1,6 +1,9 @@ ; RUN: llc -mtriple=spirv-unknown-vulkan-compute -O0 %s -o - -verify-machineinstrs | FileCheck %s --match-full-lines ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan-compute %s -o - -filetype=obj | spirv-val %} +; NOTE: Many BB have 2 reg2mem registers: one for the register usage moved +; to memory, and a second one just after caused by a PHI node. + target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1" target triple = "spirv-unknown-vulkan-compute" @@ -26,8 +29,8 @@ entry: } -; CHECK-DAG: OpName %[[#reg_0:]] "cond.reg2mem" -; CHECK-DAG: OpName %[[#reg_1:]] "cond9.reg2mem" +; CHECK-DAG: OpName %[[#a:]] "a" +; CHECK-DAG: OpName %[[#b:]] "b" define internal spir_func void @main() #0 { ; CHECK: OpSelectionMerge %[[#cond1_merge:]] None @@ -39,21 +42,27 @@ entry: br i1 true, label %cond1_true, label %cond1_false ; CHECK: %[[#cond1_true]] = OpLabel -; CHECK: OpStore %[[#reg_0]] %[[#]] +; CHECK: %[[#tmp1:]] = OpLoad %[[#]] %[[#a]] Aligned 4 +; CHECK: OpStore %[[#tmp2:]] %[[#tmp1]] Aligned 4 +; CHECK: %[[#tmp3:]] = OpLoad %[[#]] %[[#tmp2]] Aligned 4 +; CHECK: OpStore %[[#r2m:]] %[[#tmp3]] Aligned 4 ; CHECK: OpBranch %[[#cond1_merge]] cond1_true: %2 = load i32, ptr %a, align 4 br label %cond1_merge ; CHECK: %[[#cond1_false]] = OpLabel -; CHECK: OpStore %[[#reg_0]] %[[#]] +; CHECK: %[[#tmp1:]] = OpLoad %[[#]] %[[#b]] Aligned 4 +; CHECK: OpStore %[[#tmp2:]] %[[#tmp1]] Aligned 4 +; CHECK: %[[#tmp3:]] = OpLoad %[[#]] %[[#tmp2]] Aligned 4 +; CHECK: OpStore %[[#r2m]] %[[#tmp3]] Aligned 4 ; CHECK: OpBranch %[[#cond1_merge]] cond1_false: %3 = load i32, ptr %b, align 4 br label %cond1_merge ; CHECK: %[[#cond1_merge]] = OpLabel -; CHECK: %[[#tmp:]] = OpLoad %[[#]] %[[#reg_0]] +; CHECK: %[[#tmp:]] = OpLoad %[[#]] %[[#r2m]] Aligned 4 ; CHECK: %[[#cond:]] = OpINotEqual %[[#]] %[[#tmp]] %[[#]] ; CHECK: OpSelectionMerge %[[#cond2_merge:]] None ; CHECK: OpBranchConditional %[[#cond]] %[[#cond2_true:]] %[[#cond2_merge]] @@ -69,7 +78,7 @@ cond2_true: br label %cond2_merge ; CHECK: %[[#cond2_merge]] = OpLabel -; CHECK: OpFunctionCall +; CHECK: %[[#]] = OpFunctionCall %[[#]] %[[#]] ; CHECK: OpSelectionMerge %[[#cond3_merge:]] None ; CHECK: OpBranchConditional %[[#]] %[[#cond3_true:]] %[[#cond3_false:]] cond2_merge: @@ -77,24 +86,28 @@ cond2_merge: br i1 true, label %cond3_true, label %cond3_false ; CHECK: %[[#cond3_true]] = OpLabel -; CHECK: OpFunctionCall -; CHECK: OpStore %[[#reg_1]] %[[#]] +; CHECK: %[[#tmp1:]] = OpFunctionCall %[[#]] %[[#]] +; CHECK: OpStore %[[#tmp2:]] %[[#tmp1]] Aligned 4 +; CHECK: %[[#tmp3:]] = OpLoad %[[#]] %[[#tmp2]] Aligned 4 +; CHECK: OpStore %[[#r2m2:]] %[[#tmp3]] Aligned 4 ; CHECK: OpBranch %[[#cond3_merge]] cond3_true: %call5 = call spir_func noundef i32 @fn1() #4 [ "convergencectrl"(token %0) ] br label %cond3_merge ; CHECK: %[[#cond3_false]] = OpLabel -; CHECK: OpFunctionCall -; CHECK: OpStore %[[#reg_1]] %[[#]] +; CHECK: %[[#tmp1:]] = OpFunctionCall %[[#]] %[[#]] +; CHECK: OpStore %[[#tmp2:]] %[[#tmp1]] Aligned 4 +; CHECK: %[[#tmp3:]] = OpLoad %[[#]] %[[#tmp2]] Aligned 4 +; CHECK: OpStore %[[#r2m2]] %[[#tmp3]] Aligned 4 ; CHECK: OpBranch %[[#cond3_merge]] cond3_false: %call7 = call spir_func noundef i32 @fn2() #4 [ "convergencectrl"(token %0) ] br label %cond3_merge ; CHECK: %[[#cond3_merge]] = OpLabel -; CHECK: %[[#tmp:]] = OpLoad %[[#]] %[[#reg_1]] -; CHECK: %[[#cond:]] = OpINotEqual %[[#]] %[[#tmp]] %[[#]] +; CHECK: %[[#tmp:]] = OpLoad %[[#]] %[[#r2m2]] Aligned 4 +; CHECK: %[[#cond:]] = OpINotEqual %[[#]] %[[#tmp]] %[[#]] ; CHECK: OpSelectionMerge %[[#cond4_merge:]] None ; CHECK: OpBranchConditional %[[#cond]] %[[#cond4_true:]] %[[#cond4_merge]] cond3_merge: