[llvm] [SPIRV] Add a `SPIRVTypeInst` type with some guardrails (PR #179947)

[via llvm-commits]

Juan Manuel Martinez =?utf-8?q?Caamaño?=,
Juan Manuel Martinez =?utf-8?q?Caamaño?Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/179947 at github.com>


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Juan Manuel Martinez Caamaño (jmmartinez)

<details>
<summary>Changes</summary>

The idea behind this PR is to propose a type that we can deploy gradually to add some guardrails and enforce invariants in the SPIRV backend.

The PR has 3 commits:
* A first commit where the `SPIRVTypeInst` type is proposed. It's just a wrapper around `MachineInstr` that adds an assert to check that a `SPIRVTypeInst` defines a register with the type register class.
* A second commit that shows how the migration could look like for a single function.
* A third commit that motivates why: we have a `SPIRVType *TypeInst` that never defines a type in a function whose intention looks very confusing.

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


2 Files Affected:

- (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h (+23) 
- (modified) llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp (+11-5) 


``````````diff
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
index fa45f169f8561..a9b0abb385ec8 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
@@ -28,6 +28,29 @@ class SPIRVSubtarget;
 using SPIRVType = const MachineInstr;
 using StructOffsetDecorator = std::function<void(Register)>;
 
+class SPIRVTypeInst {
+  const MachineInstr *MI;
+
+public:
+  static bool definesATypeRegister(const MachineInstr *MI) {
+    const MachineRegisterInfo &MRI = MI->getMF()->getRegInfo();
+    return MRI.getRegClass(MI->getOperand(0).getReg()) == &SPIRV::TYPERegClass;
+  }
+
+  SPIRVTypeInst(const MachineInstr &MI) : SPIRVTypeInst(&MI) {}
+  SPIRVTypeInst(const MachineInstr *MI) : MI(MI) {
+    // A SPIRV Type whose result is not a type is invalid.
+    assert(definesATypeRegister(MI));
+  }
+
+  const MachineInstr &operator*() const { return *MI; }
+  const MachineInstr *operator->() const { return MI; }
+  operator const MachineInstr *() const { return MI; }
+
+  bool operator==(const SPIRVTypeInst &Other) const { return MI == Other.MI; }
+  bool operator!=(const SPIRVTypeInst &Other) const { return MI != Other.MI; }
+};
+
 class SPIRVGlobalRegistry : public SPIRVIRMapping {
   // Registers holding values which have types associated with them.
   // Initialized upon VReg definition in IRTranslator.
diff --git a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
index 3e5ce4b90ea4a..f2692660b3476 100644
--- a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
@@ -30,7 +30,8 @@ SPIRVTargetLowering::SPIRVTargetLowering(const TargetMachine &TM,
 
 // Returns true of the types logically match, as defined in
 // https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpCopyLogical.
-static bool typesLogicallyMatch(const SPIRVType *Ty1, const SPIRVType *Ty2,
+static bool typesLogicallyMatch(const SPIRVTypeInst Ty1,
+                                const SPIRVTypeInst Ty2,
                                 SPIRVGlobalRegistry &GR) {
   if (Ty1->getOpcode() != Ty2->getOpcode())
     return false;
@@ -43,17 +44,19 @@ static bool typesLogicallyMatch(const SPIRVType *Ty1, const SPIRVType *Ty2,
     if (Ty1->getOperand(2).getReg() != Ty2->getOperand(2).getReg())
       return false;
 
-    SPIRVType *ElemType1 = GR.getSPIRVTypeForVReg(Ty1->getOperand(1).getReg());
-    SPIRVType *ElemType2 = GR.getSPIRVTypeForVReg(Ty2->getOperand(1).getReg());
+    SPIRVTypeInst ElemType1 =
+        GR.getSPIRVTypeForVReg(Ty1->getOperand(1).getReg());
+    SPIRVTypeInst ElemType2 =
+        GR.getSPIRVTypeForVReg(Ty2->getOperand(1).getReg());
     return ElemType1 == ElemType2 ||
            typesLogicallyMatch(ElemType1, ElemType2, GR);
   }
 
   if (Ty1->getOpcode() == SPIRV::OpTypeStruct) {
     for (unsigned I = 1; I < Ty1->getNumOperands(); I++) {
-      SPIRVType *ElemType1 =
+      SPIRVTypeInst ElemType1 =
           GR.getSPIRVTypeForVReg(Ty1->getOperand(I).getReg());
-      SPIRVType *ElemType2 =
+      SPIRVTypeInst ElemType2 =
           GR.getSPIRVTypeForVReg(Ty2->getOperand(I).getReg());
       if (ElemType1 != ElemType2 &&
           !typesLogicallyMatch(ElemType1, ElemType2, GR))
@@ -140,6 +143,9 @@ SPIRVTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
 
 inline Register getTypeReg(MachineRegisterInfo *MRI, Register OpReg) {
   SPIRVType *TypeInst = MRI->getVRegDef(OpReg);
+  assert(!SPIRVTypeInst::definesATypeRegister(TypeInst) &&
+         "The TypeInst is never a type, because this function doesn't make any "
+         "sense");
   return TypeInst && TypeInst->getOpcode() == SPIRV::OpFunctionParameter
              ? TypeInst->getOperand(1).getReg()
              : OpReg;

``````````

</details>


https://github.com/llvm/llvm-project/pull/179947