[SPIR-V] Emit SPIR-V bitcasts between source/expected pointer type

[michalpaszkowski]

This patch introduces a new spv_ptrcast intrinsic for tracking expected pointer types. The change fixes multiple OpenCL CTS regressions due the switch to opaque pointers (e.g. basic/hiloeo).

[llvmbot]

@llvm/pr-subscribers-llvm-ir

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

Author: Michal Paszkowski (michalpaszkowski)

Changes

This patch introduces a new spv_ptrcast intrinsic for tracking expected pointer types. The change fixes multiple OpenCL CTS regressions due the switch to opaque pointers (e.g. basic/hiloeo).

---

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

3 Files Affected:

• (modified) llvm/include/llvm/IR/IntrinsicsSPIRV.td (+1)
• (modified) llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp (+44-2)
• (modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp (+20-6)

diff --git a/llvm/include/llvm/IR/IntrinsicsSPIRV.td b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
index 736be8ca3212bf2..ea0074d22a44195 100644
--- a/llvm/include/llvm/IR/IntrinsicsSPIRV.td
+++ b/llvm/include/llvm/IR/IntrinsicsSPIRV.td
@@ -28,6 +28,7 @@ let TargetPrefix = "spv" in {
   def int_spv_insertelt : Intrinsic<[llvm_any_ty], [llvm_any_ty, llvm_any_ty, llvm_anyint_ty]>;
   def int_spv_const_composite : Intrinsic<[llvm_i32_ty], [llvm_vararg_ty]>;
   def int_spv_bitcast : Intrinsic<[llvm_any_ty], [llvm_any_ty]>;
+  def int_spv_ptrcast : Intrinsic<[llvm_any_ty], [llvm_any_ty, llvm_metadata_ty, llvm_i32_ty], [ImmArg<ArgIndex<2>>]>;
   def int_spv_switch : Intrinsic<[], [llvm_any_ty, llvm_vararg_ty]>;
   def int_spv_cmpxchg : Intrinsic<[llvm_i32_ty], [llvm_any_ty, llvm_vararg_ty]>;
   def int_spv_unreachable : Intrinsic<[], []>;
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index 610d9a033aeea64..47fab8a76a12990 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -74,6 +74,10 @@ class SPIRVEmitIntrinsics
   void processInstrAfterVisit(Instruction *I);
   void insertAssignPtrTypeIntrs(Instruction *I);
   void insertAssignTypeIntrs(Instruction *I);
+
+  DenseSet<Instruction *> PtrCastInstrs;
+  void insertPtrCastInstr(Instruction *I);
+
   void processGlobalValue(GlobalVariable &GV);
 
 public:
@@ -255,7 +259,19 @@ Instruction *SPIRVEmitIntrinsics::visitGetElementPtrInst(GetElementPtrInst &I) {
 }
 
 Instruction *SPIRVEmitIntrinsics::visitBitCastInst(BitCastInst &I) {
-  SmallVector<Type *, 2> Types = {I.getType(), I.getOperand(0)->getType()};
+  Value *Source = I.getOperand(0);
+  
+  // SPIR-V, contrary to LLVM 17+ IR, supports bitcasts between pointers of
+  // varying element types. In case of IR coming from older versions of LLVM
+  // such bitcasts do not provide sufficient information, should be just skipped
+  // here, and handled in insertPtrCastInstr.
+  if (I.getType()->isPointerTy()) {
+    I.replaceAllUsesWith(Source);
+    I.eraseFromParent();
+    return &I;
+  }
+    
+  SmallVector<Type *, 2> Types = {I.getType(), Source->getType()};
   SmallVector<Value *> Args(I.op_begin(), I.op_end());
   auto *NewI = IRB->CreateIntrinsic(Intrinsic::spv_bitcast, {Types}, {Args});
   std::string InstName = I.hasName() ? I.getName().str() : "";
@@ -265,6 +281,30 @@ Instruction *SPIRVEmitIntrinsics::visitBitCastInst(BitCastInst &I) {
   return NewI;
 }
 
+void SPIRVEmitIntrinsics::insertPtrCastInstr(Instruction *I) {
+  StoreInst *SI = dyn_cast<StoreInst>(I);
+  if (!SI)
+    return;
+
+  // TODO: Do not emit new spv_ptrcast if equivalent one already exists or when
+  // spv_assign_ptr_type already targets this pointer with the same element type.
+
+  Value *Pointer = SI->getPointerOperand();
+  Type *ExpectedElementType = SI->getValueOperand()->getType();
+  Constant *ExpectedElementTypeConst =
+      Constant::getNullValue(ExpectedElementType);
+  ConstantAsMetadata *CM =
+      ValueAsMetadata::getConstant(ExpectedElementTypeConst);
+  MDTuple *TyMD = MDNode::get(F->getContext(), CM);
+  MetadataAsValue *VMD = MetadataAsValue::get(F->getContext(), TyMD);
+  unsigned AddressSpace = Pointer->getType()->getPointerAddressSpace();
+
+  SmallVector<Type *, 2> Types = {Pointer->getType(), Pointer->getType()};
+  SmallVector<Value *, 2> Args = {Pointer, VMD, IRB->getInt32(AddressSpace)};
+  auto *PtrCastI = IRB->CreateIntrinsic(Intrinsic::spv_ptrcast, {Types}, Args);
+  SI->setOperand(1, PtrCastI);
+}
+
 Instruction *SPIRVEmitIntrinsics::visitInsertElementInst(InsertElementInst &I) {
   SmallVector<Type *, 4> Types = {I.getType(), I.getOperand(0)->getType(),
                                   I.getOperand(1)->getType(),
@@ -524,6 +564,7 @@ bool SPIRVEmitIntrinsics::runOnFunction(Function &Func) {
   for (auto &I : Worklist) {
     insertAssignPtrTypeIntrs(I);
     insertAssignTypeIntrs(I);
+    insertPtrCastInstr(I);
   }
 
   for (auto *I : Worklist) {
@@ -531,7 +572,8 @@ bool SPIRVEmitIntrinsics::runOnFunction(Function &Func) {
     if (!I->getType()->isVoidTy() || isa<StoreInst>(I))
       IRB->SetInsertPoint(I->getNextNode());
     I = visit(*I);
-    processInstrAfterVisit(I);
+    if (I->getParent())
+      processInstrAfterVisit(I);
   }
   return true;
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index f4076be2a7b778f..c0d631fb69dee4d 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -125,12 +125,26 @@ static void insertBitcasts(MachineFunction &MF, SPIRVGlobalRegistry *GR,
   SmallVector<MachineInstr *, 10> ToErase;
   for (MachineBasicBlock &MBB : MF) {
     for (MachineInstr &MI : MBB) {
-      if (!isSpvIntrinsic(MI, Intrinsic::spv_bitcast))
-        continue;
-      assert(MI.getOperand(2).isReg());
-      MIB.setInsertPt(*MI.getParent(), MI);
-      MIB.buildBitcast(MI.getOperand(0).getReg(), MI.getOperand(2).getReg());
-      ToErase.push_back(&MI);
+      if (isSpvIntrinsic(MI, Intrinsic::spv_bitcast)) {
+        assert(MI.getOperand(2).isReg());
+        MIB.setInsertPt(*MI.getParent(), MI);
+        MIB.buildBitcast(MI.getOperand(0).getReg(), MI.getOperand(2).getReg());
+        ToErase.push_back(&MI);
+      } else if (isSpvIntrinsic(MI, Intrinsic::spv_ptrcast)) {
+        assert(MI.getOperand(2).isReg());
+        MIB.setInsertPt(*MI.getParent(), MI);
+
+        SPIRVType *BaseTy = GR->getOrCreateSPIRVType(
+            getMDOperandAsType(MI.getOperand(3).getMetadata(), 0), MIB);
+        SPIRVType *AssignedPtrType = GR->getOrCreateSPIRVPointerType(
+            BaseTy, MI, *MF.getSubtarget<SPIRVSubtarget>().getInstrInfo(),
+            addressSpaceToStorageClass(MI.getOperand(4).getImm()));
+
+        GR->assignSPIRVTypeToVReg(AssignedPtrType, MI.getOperand(0).getReg(), MF);
+        MIB.buildBitcast(MI.getOperand(0).getReg(), MI.getOperand(2).getReg());
+        ToErase.push_back(&MI);
+      }
+
     }
   }
   for (MachineInstr *MI : ToErase)

[michalpaszkowski]

The patch is WIP, apart from the one TODO in the code (which will eliminate adding unnecessary spv_ptrcast calls), I need to fix/adjust 14 LIT tests, and add a test covering the cases fixed in CTS.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[iliya-diyachkov]

Please fix minor issues.

[Keenuts]

Others have said everything I had to say.
LGTM on my end then.

[michalpaszkowski]

Fixed another issue @Keenuts found and added relevant tests.

[iliya-diyachkov]

The patch looks good to me.