Skip to content

[SPIR-V][Codegen] Add isPhi bit to MCInstrDesc/MIR definitions #110019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

Keenuts
Copy link
Contributor

@Keenuts Keenuts commented Sep 25, 2024

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

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 llvm#108844

Signed-off-by: Nathan Gauër <[email protected]>
@llvmbot
Copy link
Member

llvmbot commented Sep 25, 2024

@llvm/pr-subscribers-backend-spir-v

Author: Nathan Gauër (Keenuts)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/110019.diff

9 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachineInstr.h (+12-3)
  • (modified) llvm/include/llvm/MC/MCInstrDesc.h (+5)
  • (modified) llvm/include/llvm/Target/GenericOpcodes.td (+1)
  • (modified) llvm/include/llvm/Target/Target.td (+2)
  • (modified) llvm/lib/CodeGen/MachineVerifier.cpp (+4-2)
  • (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.td (+4-2)
  • (modified) llvm/utils/TableGen/Common/CodeGenInstruction.cpp (+1)
  • (modified) llvm/utils/TableGen/Common/CodeGenInstruction.h (+1)
  • (modified) llvm/utils/TableGen/InstrInfoEmitter.cpp (+2)
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)

@llvmbot
Copy link
Member

llvmbot commented Sep 25, 2024

@llvm/pr-subscribers-tablegen

Author: Nathan Gauër (Keenuts)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/110019.diff

9 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachineInstr.h (+12-3)
  • (modified) llvm/include/llvm/MC/MCInstrDesc.h (+5)
  • (modified) llvm/include/llvm/Target/GenericOpcodes.td (+1)
  • (modified) llvm/include/llvm/Target/Target.td (+2)
  • (modified) llvm/lib/CodeGen/MachineVerifier.cpp (+4-2)
  • (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.td (+4-2)
  • (modified) llvm/utils/TableGen/Common/CodeGenInstruction.cpp (+1)
  • (modified) llvm/utils/TableGen/Common/CodeGenInstruction.h (+1)
  • (modified) llvm/utils/TableGen/InstrInfoEmitter.cpp (+2)
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)

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does PHI end up transforming into an OpPhi? Can you directly consume generic PHI instructions, and only change the final encoding?

@@ -187,6 +187,7 @@ enum Flag {
Trap,
VariadicOpsAreDefs,
Authenticated,
Phi,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bits are precious and it would be best to avoid spending one on this

@VyacheslavLevytskyy
Copy link
Contributor

How does PHI end up transforming into an OpPhi? Can you directly consume generic PHI instructions, and only change the final encoding?

@arsenm There is a brief answer and a long one to this very good question.
PHI is transformed into OpPhi after instruction selection, and it's after "InstructionSelect" step that we observe complaints from MachineVerifier. I would expect that it's a right step to generate target-specific machine instructions (https://llvm.org/docs/CodeGenerator.html#instruction-selection-section).

I'd also expect it to be a quite non-trivial task to use generic phi to the very end of translation instead of creating them during instruction selection. I can think immediately about result type that OpPhi expects to be a part of the instruction and phi can't support by itself, and about difference of referring to predecessors:

  • It's a requirement of the SPIR-V specification (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpPhi) that "There must be exactly one Parent i for each parent block of the current block in the CFG.", but
  • phi-node should have one entry for each predecessor (meaning for each instance of the edge), so that %retval.0.i = phi i32 [ 0, %sw.default.i ], [ 1, %entry ], [ 1, %entry ] is equivalent to %retval_0_i = OpPhi %uint %uint_0 %13 %uint_1 %12 where there is just one record per 2 original %entry predecessors.

I don't say that it's impossible to implement, because it's virtually anything that can be implemented, but it definitely seems unfair to force anyone selecting between being considered invalid and develop something overcomplicated just to avoid such a misjudgment -- and that's actually a part of a longer answer. The point is that GlobalISel has rather poor support for a higher level specification that is SPIR-V. If we narrow things just to MachineVerifier, I can recall one more example, when it's impossible to satisfy MachineVerifier.

In G_BITCAST we see if (SrcTy == DstTy) report("bitcast must change the type", MI); where SrcTy and DstTy both being LLT pointers just have no means to express the notion of pointer type that is important for SPIR-V where a user may and should use bitcast between pointers with different pointee types (see, https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpBitcast).

I guess this PR is not such a good place for a long reply, but I'd like to provide you with a better context and probably to hear from you an early feedback about the wider problem, even before me and @michalpaszkowski hopefully touch this topic at the 2024 LLVM Developers’ Meeting.

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 26, 2024

Thanks @VyacheslavLevytskyy for the detailed reply! I agree, delaying translation would seems weird (the result type, and also... weird to translate all in Isel except OpPhi)

After some discussions with a colleague, looks like there is another way which spares this precious bit: adding the info into TargetInstrInfo, and using this to check the opcode/offset.
Updated the PR to match this new version. Running tests now to verify my implementation, and will update this PR.

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 26, 2024

Mmm a lot of things are starting to blow up if the OpPhi is generically accepted as a PHI instruction. (MIR optimizations relying on isPHI, but then loading operands by index, hard-coded to start at 1).

Surprisingly, this wasn't the case with the change here.
I see 2 options:

  • make MachineInstr::isPHI() return true for G_PHI, PHI and OpPhi, but this requires fixing all shared MIR passes (like TailDuplication) modifying phi nodes so they don't hard-code offsets. But we would leave non-generic code (for ex x86 backend with hard-coded values)
  • ass a MachineInstr::isLikePHI() or something like that to only fix the MachineVerifier case.

I think option 1 is better, but quite invasive. Option 2 is more safe, localized.
@arsenm, any objection to have such large-scale change?

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 27, 2024

So, looks like fixing isPHI() to read from the TargetInstrInfo requires touching to all the backends so far. Some parts already have TII, so easy fix, other don't and require more of less changes to get TII.
Getting the TII from the MachineInstr parents is almost good, except if you remove an instruction from a BB, then you don't have this context, and isPHI cannot work.

@arsenm : would it be OK to add pointer to TII in MachineInstr? This way, we could keep the isPHI in MachineInstr, and add the iterators for it: incoming_phi_operands_begin()

Keenuts added a commit to Keenuts/llvm-project that referenced this pull request Sep 30, 2024
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]>
@arsenm
Copy link
Contributor

arsenm commented Sep 30, 2024

phi-node should have one entry for each predecessor (meaning for each instance of the edge), so that %retval.0.i = phi i32 [ 0, %sw.default.i ], [ 1, %entry ], [ 1, %entry ] is equivalent to %retval_0_i = OpPhi %uint %uint_0 %13 %uint_1 %12 where there is just one record per 2 original %entry predecessors.

OK, so I think in this case, it shouldn't be treated like a phi at all. That's structurally different, and should have a different strategy. No generic transform should be treating this as if it were a phi

In G_BITCAST we see if (SrcTy == DstTy) report("bitcast must change the type", MI); where SrcTy and DstTy both being LLT pointers just have no means to express the notion of pointer type that is important for SPIR-V where a user may and should use bitcast between pointers with different pointee types (see, https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpBitcast).

Not sure what the issue here is, or how it's connected to the phi

@Keenuts
Copy link
Contributor Author

Keenuts commented Sep 30, 2024

  • %retval.0.i = phi i32 [ 0, %sw.default.i ], [ 1, %entry ], [ 1, %entry ]

This is legal? "After this, the ‘phi’ instruction takes a list of pairs as arguments, with one pair for each predecessor basic block of the current block. "
Looks like there should be 1 value per predecessor, so only 2 pairs for this instruction no?
(Even if there are 2 edges, it's still the same predecessor)

@VyacheslavLevytskyy
Copy link
Contributor

  • %retval.0.i = phi i32 [ 0, %sw.default.i ], [ 1, %entry ], [ 1, %entry ]

This is legal? "After this, the ‘phi’ instruction takes a list of pairs as arguments, with one pair for each predecessor basic block of the current block. " Looks like there should be 1 value per predecessor, so only 2 pairs for this instruction no? (Even if there are 2 edges, it's still the same predecessor)

Please have a look at KhronosGroup/SPIRV-LLVM-Translator#2702 for more context, there I've tried to describe it with details. It's exotic but legal.

@Keenuts
Copy link
Contributor Author

Keenuts commented Oct 1, 2024

Please have a look at KhronosGroup/SPIRV-LLVM-Translator#2702 for more context, there I've tried to describe it with details. It's exotic but legal.

Ahh yes, looks like it it, but if the value for the same predecessor is not consistent, then there is a diagnostic. Thanks!

PHI node has multiple entries for the same basic block with different incoming values!
  %3 = phi i32 [ 0, %left ], [ 1, %left ], [ 0, %right ]

@arsenm
Copy link
Contributor

arsenm commented Oct 2, 2024

I'd also expect it to be a quite non-trivial task to use generic phi to the very end of translation instead of creating them during instruction selection. I can think immediately about result type that OpPhi expects to be a part of the instruction and phi can't support by itself, and about difference of referring to predecessors:

I do think it will be easier to maintain generic phi to a later point, not necessarily the very end, than try to pretend your different operation is the same thing. The normal flow is to break phis into copies at the start of register allocation, I would expect the SPIRV analog to introduce whatever flavor it wants around the same point

I don't say that it's impossible to implement, because it's virtually anything that can be implemented, but it definitely seems unfair to force anyone selecting between being considered invalid and develop something overcomplicated just to avoid such a misjudgment -- and that's actually a part of a longer answer.

I think expanding the shape and requirements for what all of codegen must consider as a phi will be more complex

Keenuts added a commit to Keenuts/llvm-project that referenced this pull request Oct 29, 2024
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]>
Keenuts added a commit to Keenuts/llvm-project that referenced this pull request Nov 4, 2024
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]>
Keenuts added a commit to Keenuts/llvm-project that referenced this pull request Nov 4, 2024
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]>
@Keenuts Keenuts closed this Dec 17, 2024
@Keenuts Keenuts deleted the fix-108844 branch December 17, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIR-V] OpPhi not handled as a phi node by the MIR verifier
4 participants