[SPIRV] Print split 64-bit OpSwitch operands as a single operand for text output

[sarnex]

In binary form, 64-bit values are split into two 32-bit values as per the spec. Naturally this works fine with all tools.

However, the text format does not have a formal specification but SPIR-V-Tools, which we already rely on in the SPIRV workflow (clang calls spirv-as for example), expects the full 64 bit value, but today we print the two 32-bit values. causing the tool to error and report that the format is invalid.

The SPIR-V Translator also prints a single 64-bit value for text format.

This case is already handled specifically for OpConstant, but OpSwitch was missed. The SPIR-V translator also has special code in OpSwitch handling for this case.

Recombine the two 32-bit operands into a single 64-bit value to print in AsmPrinter. The actual ASM (aka binary form) emission is unchanged.

[llvmbot]

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

Author: Nick Sarnie (sarnex)

Changes

In binary form, 64-bit values are split into two 32-bit values as per the spec. Naturally this works fine with all tools.

However, the text format does not have a formal specification but SPIR-V-Tools, which we already rely on in the SPIRV workflow (clang calls spirv-as for example), expects the full 64 bit value, but today we print the two 32-bit values. causing the tool to error and report that the format is invalid.

The SPIR-V Translator also prints a single 64-bit value for text format.

This case is already handled specifically for OpConstant, but OpSwitch was missed. The SPIR-V translator also has special code in OpSwitch handling for this case.

Recombine the two 32-bit operands into a single 64-bit value to print in AsmPrinter. The actual ASM (aka binary form) emission is unchanged.

---

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

6 Files Affected:

• (modified) llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h (+4-1)
• (modified) llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp (+29)
• (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.h (+3-1)
• (modified) llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp (+2)
• (modified) llvm/lib/Target/SPIRV/SPIRVUtils.cpp (+2)
• (modified) llvm/test/CodeGen/SPIRV/branching/OpSwitch64.ll (+5-1)

diff --git a/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h b/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
index d76180ce97e9e..ea41716bf204e 100644
--- a/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
+++ b/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h
@@ -245,7 +245,10 @@ struct ExtendedBuiltin {
 
 enum InstFlags {
   // It is a half type
-  INST_PRINTER_WIDTH16 = 1
+  INST_PRINTER_WIDTH16 = 1,
+  // It is a 64-bit type
+  INST_PRINTER_WIDTH64 = INST_PRINTER_WIDTH16 << 1,
+
 };
 } // namespace SPIRV
 
diff --git a/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp b/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp
index 35a2ee16b2309..6a0f5b70e5d67 100644
--- a/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp
+++ b/llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVInstPrinter.cpp
@@ -167,6 +167,35 @@ void SPIRVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
               MI, FirstVariableIndex, OS);
           printRemainingVariableOps(MI, FirstVariableIndex + 1, OS);
           break;
+        case SPIRV::OpSwitch:
+          if (MI->getFlags() & SPIRV::INST_PRINTER_WIDTH64) {
+            // In binary format 64-bit types are split into two 32-bit operands,
+            // but in text format combine these into a single 64-bit value as
+            // this is what tools such as spirv-as require.
+            const unsigned NumOps = MI->getNumOperands();
+            for (unsigned OpIdx = NumFixedOps; OpIdx < NumOps;) {
+              if (OpIdx + 1 >= NumOps || !MI->getOperand(OpIdx).isImm() ||
+                  !MI->getOperand(OpIdx + 1).isImm()) {
+                continue;
+              }
+              OS << ' ';
+              uint64_t LowBits = MI->getOperand(OpIdx).getImm();
+              uint64_t HighBits = MI->getOperand(OpIdx + 1).getImm();
+              uint64_t CombinedValue = (HighBits << 32) | LowBits;
+              OS << formatImm(CombinedValue);
+              OpIdx += 2;
+
+              // Next should be the label
+              if (OpIdx < NumOps) {
+                OS << ' ';
+                printOperand(MI, OpIdx, OS);
+                OpIdx++;
+              }
+            }
+          } else {
+            printRemainingVariableOps(MI, NumFixedOps, OS);
+          }
+          break;
         case SPIRV::OpImageSampleImplicitLod:
         case SPIRV::OpImageSampleDrefImplicitLod:
         case SPIRV::OpImageSampleProjImplicitLod:
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
index 4de9d6a936abd..4c5b81fdd359a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.h
@@ -62,7 +62,9 @@ class SPIRVInstrInfo : public SPIRVGenInstrInfo {
 namespace SPIRV {
 enum AsmComments {
   // It is a half type
-  ASM_PRINTER_WIDTH16 = MachineInstr::TAsmComments
+  ASM_PRINTER_WIDTH16 = MachineInstr::TAsmComments,
+  // It is a 64 bit type
+  ASM_PRINTER_WIDTH64 = ASM_PRINTER_WIDTH16 << 1,
 };
 } // namespace SPIRV
 
diff --git a/llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp b/llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp
index e39666c5bc345..9aa07b52b4081 100644
--- a/llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVMCInstLower.cpp
@@ -25,6 +25,8 @@ void SPIRVMCInstLower::lower(const MachineInstr *MI, MCInst &OutMI,
   // Propagate previously set flags
   if (MI->getAsmPrinterFlags() & SPIRV::ASM_PRINTER_WIDTH16)
     OutMI.setFlags(SPIRV::INST_PRINTER_WIDTH16);
+  if (MI->getAsmPrinterFlags() & SPIRV::ASM_PRINTER_WIDTH64)
+    OutMI.setFlags(SPIRV::INST_PRINTER_WIDTH64);
   const MachineFunction *MF = MI->getMF();
   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = MI->getOperand(i);
diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
index 4e2cc882ed6ba..8f2fc01da476f 100644
--- a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
@@ -105,6 +105,8 @@ void addNumImm(const APInt &Imm, MachineInstrBuilder &MIB) {
     uint32_t LowBits = FullImm & 0xffffffff;
     uint32_t HighBits = (FullImm >> 32) & 0xffffffff;
     MIB.addImm(LowBits).addImm(HighBits);
+    // Asm Printer needs this info to print 64-bit operands correctly
+    MIB.getInstr()->setAsmPrinterFlag(SPIRV::ASM_PRINTER_WIDTH64);
     return;
   }
   report_fatal_error("Unsupported constant bitwidth");
diff --git a/llvm/test/CodeGen/SPIRV/branching/OpSwitch64.ll b/llvm/test/CodeGen/SPIRV/branching/OpSwitch64.ll
index 5e4f1f14f8f91..8fc986fe41b44 100644
--- a/llvm/test/CodeGen/SPIRV/branching/OpSwitch64.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/OpSwitch64.ll
@@ -19,7 +19,11 @@
 
 ; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
 
-; CHECK-SPIRV: OpSwitch %[[#]] %[[#]] 0 0 %[[#]] 1 0 %[[#]] 1 5 %[[#]]
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=asm | spirv-as - -o /dev/null %}
+
+; CHECK-SPIRV: OpSwitch %[[#]] %[[#]] 0 %[[#]] 1 %[[#]] 21474836481 %[[#]]
 
 define spir_kernel void @test_64(i32 addrspace(1)* %res) {
 entry:

[jmmartinez]

Looks good to me, but please wait for a second reviewer's approval since I'm fairly new to SPIRV.

[sarnex]

Thank you!