From 86375a2cdaeccac8201a7c1cf81963ade8cef14c Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Wed, 13 Sep 2023 16:38:35 +0200 Subject: [PATCH 1/7] [SPIRV] Add support for SPV_KHR_bit_instructions Draft version of patch to add support for SPV_KHR_bit_instructions. Created revision so we can discuss how to proceed here. SPV_KHR_bit_instructions (http://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_bit_instructions.html), adds support for instructions that we already support through the Shader capability but without requiring the shader cap. However we currently have no way to disable the shader cap afaiu. On the other hand, SPV_KHR_bit_instructions was one of the extensions @mpaszkowski listed that we need to implement. So what does it mean at the moment the need to implement this? Is it just the ability to issue the extension and capabilities instructions without issuing the Shader capability? If so, then I will need to add here a way to disable the Shader capability. What do you think? Differential Revision: https://reviews.llvm.org/D156661 --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 19 +++++++++++++++++++ llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h | 4 ++++ llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp | 1 + .../lib/Target/SPIRV/SPIRVSymbolicOperands.td | 1 + .../SPIRV/transcoding/OpBitReverse_i32.ll | 3 +++ 5 files changed, 28 insertions(+) diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index 3754f57ef3ac7..ec713c547b132 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -15,6 +15,8 @@ //===----------------------------------------------------------------------===// #include "SPIRVModuleAnalysis.h" +#include "MCTargetDesc/SPIRVBaseInfo.h" +#include "MCTargetDesc/SPIRVMCTargetDesc.h" #include "SPIRV.h" #include "SPIRVSubtarget.h" #include "SPIRVTargetMachine.h" @@ -523,6 +525,12 @@ void SPIRV::RequirementHandler::addAvailableCaps(const CapabilityList &ToAdd) { SPIRV::OperandCategory::CapabilityOperand, Cap)); } +void SPIRV::RequirementHandler::removeCapabilityIf(const Capability::Capability ToRemove, + const Capability::Capability IfPresent) { + if (AvailableCaps.contains(IfPresent)) + AvailableCaps.erase(ToRemove); +} + namespace llvm { namespace SPIRV { void RequirementHandler::initAvailableCapabilities(const SPIRVSubtarget &ST) { @@ -734,6 +742,11 @@ void addInstrRequirements(const MachineInstr &MI, break; } case SPIRV::OpBitReverse: + case SPIRV::OpBitFieldInsert: + case SPIRV::OpBitFieldSExtract: + case SPIRV::OpBitFieldUExtract: + Reqs.addExtension(SPIRV::Extension::SPV_KHR_bit_instructions); + break; case SPIRV::OpTypeRuntimeArray: Reqs.addCapability(SPIRV::Capability::Shader); break; @@ -887,6 +900,12 @@ void addInstrRequirements(const MachineInstr &MI, default: break; } + + // If we require capability Shader, then we can remove the requirement for + // the BitInstructions capability, since Shader is a superset capability + // of BitInstructions. + Reqs.removeCapabilityIf(SPIRV::Capability::Shader, + SPIRV::Capability::BitInstructions); } static void collectReqs(const Module &M, SPIRV::ModuleAnalysisInfo &MAI, diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h index b57a5f4c28278..9eea9ac5bda5b 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.h @@ -113,6 +113,10 @@ struct RequirementHandler { bool isCapabilityAvailable(Capability::Capability Cap) const { return AvailableCaps.contains(Cap); } + + // Remove capability ToRemove, but only if IfPresent is present. + void removeCapabilityIf(const Capability::Capability ToRemove, + const Capability::Capability IfPresent); }; using InstrList = SmallVector; diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp index 1bad1d8109623..fdf103941f4b9 100644 --- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp @@ -11,6 +11,7 @@ //===----------------------------------------------------------------------===// #include "SPIRVSubtarget.h" +#include "MCTargetDesc/SPIRVBaseInfo.h" #include "SPIRV.h" #include "SPIRVGlobalRegistry.h" #include "SPIRVLegalizerInfo.h" diff --git a/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td b/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td index c27d2826c0159..ab06f5308700b 100644 --- a/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td +++ b/llvm/lib/Target/SPIRV/SPIRVSymbolicOperands.td @@ -450,6 +450,7 @@ defm PhysicalStorageBufferAddressesEXT : CapabilityOperand<5347, 0, 0, [], [Shad defm CooperativeMatrixNV : CapabilityOperand<5357, 0, 0, [], [Shader]>; defm ArbitraryPrecisionIntegersINTEL : CapabilityOperand<5844, 0, 0, [SPV_INTEL_arbitrary_precision_integers], [Int8, Int16]>; defm OptNoneINTEL : CapabilityOperand<6094, 0, 0, [SPV_INTEL_optnone], []>; +defm BitInstructions : CapabilityOperand<6025, 0, 0, [SPV_KHR_bit_instructions], []>; //===----------------------------------------------------------------------===// // Multiclass used to define SourceLanguage enum values and at the same time diff --git a/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll b/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll index f396b5a01ae91..4f95c47707804 100644 --- a/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll +++ b/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll @@ -1,5 +1,8 @@ ; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV +; CHECK-SPIRV: OpCapability BitInstructions +; CHECK-SPIRV-NEXT: OpExtension "SPV_KHR_bit_instructions" + ; CHECK-SPIRV: %[[#int:]] = OpTypeInt 32 ; CHECK-SPIRV: OpBitReverse %[[#int]] From 7cfad0278cac91fcec1a1eada5382f54d07396c4 Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 15 Sep 2023 16:55:04 +0200 Subject: [PATCH 2/7] Add test and enable SPV_KHR_bit_instructions --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 7 +++++- llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp | 6 ++++- .../extensions/SPV_KHR_bit_instructions.ll | 24 +++++++++++++++++++ .../SPIRV/transcoding/OpBitReverse_i32.ll | 3 --- 4 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions.ll diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index ec713c547b132..a4160f1a0385b 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -506,7 +506,7 @@ void SPIRV::RequirementHandler::checkSatisfiable( for (auto Ext : AllExtensions) { if (ST.canUseExtension(Ext)) continue; - LLVM_DEBUG(dbgs() << "Extension not suported: " + LLVM_DEBUG(dbgs() << "Extension not supported: " << getSymbolicOperandMnemonic( OperandCategory::ExtensionOperand, Ext) << "\n"); @@ -745,7 +745,12 @@ void addInstrRequirements(const MachineInstr &MI, case SPIRV::OpBitFieldInsert: case SPIRV::OpBitFieldSExtract: case SPIRV::OpBitFieldUExtract: + if (!ST.canUseExtension(SPIRV::Extension::SPV_KHR_bit_instructions)) { + Reqs.addCapability(SPIRV::Capability::Shader); + break; + } Reqs.addExtension(SPIRV::Extension::SPV_KHR_bit_instructions); + Reqs.addCapability(SPIRV::Capability::BitInstructions); break; case SPIRV::OpTypeRuntimeArray: Reqs.addCapability(SPIRV::Capability::Shader); diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp index fdf103941f4b9..1584ed477fa4f 100644 --- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp @@ -41,7 +41,11 @@ cl::list Extensions( clEnumValN(SPIRV::Extension::SPV_KHR_no_integer_wrap_decoration, "SPV_KHR_no_integer_wrap_decoration", "Adds decorations to indicate that a given instruction does " - "not cause integer wrapping"))); + "not cause integer wrapping"), + clEnumValN(SPIRV::Extension::SPV_KHR_bit_instructions, + "SPV_KHR_bit_instructions", + "This enables bit instructions to be used by SPIR-V modules " + "without the requiring the Shader capability"))); // Compare version numbers, but allow 0 to mean unspecified. static bool isAtLeastVer(uint32_t Target, uint32_t VerToCompareTo) { diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions.ll new file mode 100644 index 0000000000000..95395d5efb55d --- /dev/null +++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions.ll @@ -0,0 +1,24 @@ +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s --spirv-extensions=SPV_KHR_bit_instructions -o - | FileCheck %s --check-prefix=CHECK-EXTENSION +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-NO-EXTENSION + +; CHECK-EXTENSION: OpCapability BitInstructions +; CHECK-EXTENSION-NEXT: OpExtension "SPV_KHR_bit_instructions" +; CHECK-EXTENSION-NOT: OpCabilitity Shader +; CHECK-NO-EXTENSION: OpCapability Shader +; CHECK-NO-EXTENSION-NOT: OpCabilitity BitInstructions +; CHECK-NO-EXTENSION-NOT: OpExtension "SPV_KHR_bit_instructions" + + +; CHECK-EXTENSION: %[[#int:]] = OpTypeInt 32 +; CHECK-EXTENSION: OpBitReverse %[[#int]] +; CHECK-NO-EXTENSION: %[[#int:]] = OpTypeInt 32 +; CHECK-NO-EXTENSION: OpBitReverse %[[#int]] + +define spir_kernel void @testBitRev(i32 %a, i32 %b, i32 %c, i32 addrspace(1)* nocapture %res) local_unnamed_addr { +entry: + %call = tail call i32 @llvm.bitreverse.i32(i32 %b) + store i32 %call, i32 addrspace(1)* %res, align 4 + ret void +} + +declare i32 @llvm.bitreverse.i32(i32) diff --git a/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll b/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll index 4f95c47707804..f396b5a01ae91 100644 --- a/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll +++ b/llvm/test/CodeGen/SPIRV/transcoding/OpBitReverse_i32.ll @@ -1,8 +1,5 @@ ; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV -; CHECK-SPIRV: OpCapability BitInstructions -; CHECK-SPIRV-NEXT: OpExtension "SPV_KHR_bit_instructions" - ; CHECK-SPIRV: %[[#int:]] = OpTypeInt 32 ; CHECK-SPIRV: OpBitReverse %[[#int]] From 980252c159d722860f5914461ada5099b2f4a10e Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 22 Sep 2023 11:38:48 +0200 Subject: [PATCH 3/7] Remove BitInstructions if Shader is present --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index a4160f1a0385b..03891f4c815e0 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -909,8 +909,7 @@ void addInstrRequirements(const MachineInstr &MI, // If we require capability Shader, then we can remove the requirement for // the BitInstructions capability, since Shader is a superset capability // of BitInstructions. - Reqs.removeCapabilityIf(SPIRV::Capability::Shader, - SPIRV::Capability::BitInstructions); + Reqs.removeCapabilityIf(SPIRV::Capability::BitInstructions, SPIRV::Capability::Shader); } static void collectReqs(const Module &M, SPIRV::ModuleAnalysisInfo &MAI, From a5db350d1c1db8fe3475612d2118b37d5f25da22 Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 22 Sep 2023 12:22:50 +0200 Subject: [PATCH 4/7] clang-format patch --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index 03891f4c815e0..fb38ac3dd3705 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -909,7 +909,8 @@ void addInstrRequirements(const MachineInstr &MI, // If we require capability Shader, then we can remove the requirement for // the BitInstructions capability, since Shader is a superset capability // of BitInstructions. - Reqs.removeCapabilityIf(SPIRV::Capability::BitInstructions, SPIRV::Capability::Shader); + Reqs.removeCapabilityIf(SPIRV::Capability::BitInstructions, + SPIRV::Capability::Shader); } static void collectReqs(const Module &M, SPIRV::ModuleAnalysisInfo &MAI, From c69065e34bc6df06042bdc8da4d2396dd42e7081 Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 22 Sep 2023 12:24:11 +0200 Subject: [PATCH 5/7] clang-format missing hunk --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index fb38ac3dd3705..72162204afc82 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -525,8 +525,9 @@ void SPIRV::RequirementHandler::addAvailableCaps(const CapabilityList &ToAdd) { SPIRV::OperandCategory::CapabilityOperand, Cap)); } -void SPIRV::RequirementHandler::removeCapabilityIf(const Capability::Capability ToRemove, - const Capability::Capability IfPresent) { +void SPIRV::RequirementHandler::removeCapabilityIf( + const Capability::Capability ToRemove, + const Capability::Capability IfPresent) { if (AvailableCaps.contains(IfPresent)) AvailableCaps.erase(ToRemove); } From 61fd18c444d1e5ea59b4f7d1346c38158b6ab803 Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 22 Sep 2023 12:47:44 +0200 Subject: [PATCH 6/7] Rerun clang-format --- llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp index 72162204afc82..66deb6fe42fcb 100644 --- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp @@ -526,7 +526,7 @@ void SPIRV::RequirementHandler::addAvailableCaps(const CapabilityList &ToAdd) { } void SPIRV::RequirementHandler::removeCapabilityIf( - const Capability::Capability ToRemove, + const Capability::Capability ToRemove, const Capability::Capability IfPresent) { if (AvailableCaps.contains(IfPresent)) AvailableCaps.erase(ToRemove); @@ -910,7 +910,7 @@ void addInstrRequirements(const MachineInstr &MI, // If we require capability Shader, then we can remove the requirement for // the BitInstructions capability, since Shader is a superset capability // of BitInstructions. - Reqs.removeCapabilityIf(SPIRV::Capability::BitInstructions, + Reqs.removeCapabilityIf(SPIRV::Capability::BitInstructions, SPIRV::Capability::Shader); } From 6663b35fbba62ba085b28e258b4b29af41b85450 Mon Sep 17 00:00:00 2001 From: Paulo Matos Date: Fri, 22 Sep 2023 14:42:40 +0200 Subject: [PATCH 7/7] Remove extra 'the' in the option description --- llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp index 1584ed477fa4f..e4ad805f071b9 100644 --- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp +++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp @@ -45,7 +45,7 @@ cl::list Extensions( clEnumValN(SPIRV::Extension::SPV_KHR_bit_instructions, "SPV_KHR_bit_instructions", "This enables bit instructions to be used by SPIR-V modules " - "without the requiring the Shader capability"))); + "without requiring the Shader capability"))); // Compare version numbers, but allow 0 to mean unspecified. static bool isAtLeastVer(uint32_t Target, uint32_t VerToCompareTo) {