pr feedback

Signed-off-by: Nathan Gauër <[email protected]>

Author: Keenuts

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.

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))

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);
 }
 

llvm/test/MachineVerifier/SPIRV/lit.local.cfg

@@ -0,0 +1,2 @@
+if not "SPIRV" in config.root.targets:
+    config.unsupported = True

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
+...
+---

llvm/test/MachineVerifier/verifier-phi-spirv.mir renamed to 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