[SPIR-V] Do instruction selection for G_BITCAST on an earlier stage

[VyacheslavLevytskyy]

This PR implements instruction selection for G_BITCAST on an earlier stage to avoid MachineVerifier complains on subtle semantics difference between G_BITCAST and OpBitcast.

We do instruction selections for OpBitcast after IR Translation instead of calling MIB.buildBitcast() generating the general op code G_BITCAST, because when MachineVerifier validates G_BITCAST we see a check of a kind: 'if Source Type is equal to Destination Type then report error "bitcast must change the type"'. This doesn't take into account the notion of a typed pointer that is important for SPIR-V where a user may and should use bitcast between pointers with different pointee types (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpBitcast).

It's important for correct lowering in SPIR-V, because interpretation of the data type is not left to instructions that utilize the pointer, but encoded by the pointer declaration, and the SPIRV target can and must handle the declaration and use of pointers that specify the type of data they point to.

It's not feasible to improve validation of G_BITCAST using just information provided by low level types of source and destination. Therefore we don't produce G_BITCAST as the general op code with semantics different from OpBitcast, but rather lower to OpBitcast immediately.

See discussion in #110270 for even more context.

[llvmbot]

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

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

This PR implements instruction selection for G_BITCAST on an earlier stage to avoid MachineVerifier complains on subtle semantics difference between G_BITCAST and OpBitcast.

We do instruction selections for OpBitcast after IR Translation instead of calling MIB.buildBitcast() generating the general op code G_BITCAST, because when MachineVerifier validates G_BITCAST we see a check of a kind: 'if Source Type is equal to Destination Type then report error "bitcast must change the type"'. This doesn't take into account the notion of a typed pointer that is important for SPIR-V where a user may and should use bitcast between pointers with different pointee types (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpBitcast).

It's important for correct lowering in SPIR-V, because interpretation of the data type is not left to instructions that utilize the pointer, but encoded by the pointer declaration, and the SPIRV target can and must handle the declaration and use of pointers that specify the type of data they point to.

It's not feasible to improve validation of G_BITCAST using just information provided by low level types of source and destination. Therefore we don't produce G_BITCAST as the general op code with semantics different from OpBitcast, but rather lower to OpBitcast immediately.

See discussion in #110270 for even more context.

---

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

3 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp (+52-9)
• (modified) llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types-rev.ll (+1-4)
• (modified) llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types.ll (+1-4)

diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index 3c2af34dd55239..cc34cf877dea97 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -165,6 +165,57 @@ static MachineInstr *findAssignTypeInstr(Register Reg,
   return nullptr;
 }
 
+static void buildOpBitcast(SPIRVGlobalRegistry *GR, MachineIRBuilder &MIB,
+                           Register ResVReg, Register OpReg) {
+  SPIRVType *ResType = GR->getSPIRVTypeForVReg(ResVReg);
+  SPIRVType *OpType = GR->getSPIRVTypeForVReg(OpReg);
+  assert(ResType && OpType && "Operand types are expected");
+  if (!GR->isBitcastCompatible(ResType, OpType))
+    report_fatal_error("incompatible result and operand types in a bitcast");
+  MachineRegisterInfo *MRI = MIB.getMRI();
+  if (!MRI->getRegClassOrNull(ResVReg))
+    MRI->setRegClass(ResVReg, GR->getRegClass(ResType));
+  MIB.buildInstr(SPIRV::OpBitcast)
+      .addDef(ResVReg)
+      .addUse(GR->getSPIRVTypeID(ResType))
+      .addUse(OpReg);
+}
+
+// We do instruction selections early instead of calling MIB.buildBitcast()
+// generating the general op code G_BITCAST. When MachineVerifier validates
+// G_BITCAST we see a check of a kind: if Source Type is equal to Destination
+// Type then report error "bitcast must change the type". This doesn't take into
+// account the notion of a typed pointer that is important for SPIR-V where a
+// user may and should use bitcast between pointers with different pointee types
+// (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpBitcast).
+// It's important for correct lowering in SPIR-V, because interpretation of the
+// data type is not left to instructions that utilize the pointer, but encoded
+// by the pointer declaration, and the SPIRV target can and must handle the
+// declaration and use of pointers that specify the type of data they point to.
+// It's not feasible to improve validation of G_BITCAST using just information
+// provided by low level types of source and destination. Therefore we don't
+// produce G_BITCAST as the general op code with semantics different from
+// OpBitcast, but rather lower to OpBitcast immediately. As for now, the only
+// difference would be that CombinerHelper couldn't transform known patterns
+// around G_BUILD_VECTOR. See discussion
+// in https://github.com/llvm/llvm-project/pull/110270 for even more context.
+static void selectOpBitcasts(MachineFunction &MF, SPIRVGlobalRegistry *GR,
+                             MachineIRBuilder MIB) {
+  SmallVector<MachineInstr *, 16> ToErase;
+  for (MachineBasicBlock &MBB : MF) {
+    for (MachineInstr &MI : MBB) {
+      if (MI.getOpcode() != TargetOpcode::G_BITCAST)
+        continue;
+      MIB.setInsertPt(*MI.getParent(), MI);
+      buildOpBitcast(GR, MIB, MI.getOperand(0).getReg(),
+                     MI.getOperand(1).getReg());
+      ToErase.push_back(&MI);
+    }
+  }
+  for (MachineInstr *MI : ToErase)
+    MI->eraseFromParent();
+}
+
 static void insertBitcasts(MachineFunction &MF, SPIRVGlobalRegistry *GR,
                            MachineIRBuilder MIB) {
   // Get access to information about available extensions
@@ -202,15 +253,6 @@ static void insertBitcasts(MachineFunction &MF, SPIRVGlobalRegistry *GR,
       } else {
         GR->assignSPIRVTypeToVReg(AssignedPtrType, Def, MF);
         MIB.buildBitcast(Def, Source);
-        // MachineVerifier requires that bitcast must change the type.
-        // Change AddressSpace if needed to hint that Def and Source points to
-        // different types: this doesn't change actual code generation.
-        LLT DefType = MRI->getType(Def);
-        if (DefType == MRI->getType(Source))
-          MRI->setType(Def,
-                       LLT::pointer((DefType.getAddressSpace() + 1) %
-                                        SPIRVSubtarget::MaxLegalAddressSpace,
-                                    GR->getPointerSize()));
       }
     }
   }
@@ -1007,6 +1049,7 @@ bool SPIRVPreLegalizer::runOnMachineFunction(MachineFunction &MF) {
   removeImplicitFallthroughs(MF, MIB);
   insertSpirvDecorations(MF, MIB);
   insertInlineAsm(MF, GR, ST, MIB);
+  selectOpBitcasts(MF, GR, MIB);
 
   return true;
 }
diff --git a/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types-rev.ll b/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types-rev.ll
index 6fa3f4e53cc598..8d14c3a359963f 100644
--- a/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types-rev.ll
+++ b/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types-rev.ll
@@ -1,7 +1,4 @@
-; The goal of the test case is to ensure that OpPhi is consistent with respect to operand types.
-; -verify-machineinstrs is not available due to mutually exclusive requirements for G_BITCAST and G_PHI.
-
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
 ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
 
 ; CHECK: %[[#Char:]] = OpTypeInt 8 0
diff --git a/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types.ll b/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types.ll
index 4fbaae25567300..07824d4ed6cd85 100644
--- a/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types.ll
+++ b/llvm/test/CodeGen/SPIRV/pointers/phi-valid-operand-types.ll
@@ -1,7 +1,4 @@
-; The goal of the test case is to ensure that OpPhi is consistent with respect to operand types.
-; -verify-machineinstrs is not available due to mutually exclusive requirements for G_BITCAST and G_PHI.
-
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
 ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
 
 ; CHECK: %[[#Char:]] = OpTypeInt 8 0

[VyacheslavLevytskyy]

@Keenuts This should fix all those TODO(pull/110270): verifier, fix G_BITCAST error "bitcast must change type" in #111026