Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIRV] Add support for cl_khr_extended_bit_ops #120571

Merged
merged 11 commits into from
Feb 11, 2025
Merged

[SPIRV] Add support for cl_khr_extended_bit_ops #120571

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
95252e2
[SPIR-V] Add support for cl_khr_extended_bit_ops extension
maarquitos14 Nov 27, 2024
debfc1f
Change test from .ll file to .cl file.
maarquitos14 Dec 18, 2024
cc61409
Add __spirv_CODE wrappers for extended bit instructions.
maarquitos14 Dec 18, 2024
8a11b97
Undo changes on higher level directory LIT config.
maarquitos14 Dec 19, 2024
54128bb
Fix clang-format issues.
maarquitos14 Dec 19, 2024
c3fe999
Address code review feedback.
maarquitos14 Dec 20, 2024
c8b26ea
Add tests for SPIRV-friendly builtins.
maarquitos14 Jan 24, 2025
936cea7
Fix SPIRV-friendly builtins.
maarquitos14 Jan 24, 2025
aa2fd7a
Add new tests to check enabled capabilities.
maarquitos14 Jan 24, 2025
775d4ea
Update test.
maarquitos14 Jan 24, 2025
ab0ef11
Spin off SPIRV-friendly builtins test.
maarquitos14 Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,38 @@ static bool buildBarrierInst(const SPIRV::IncomingCall *Call, unsigned Opcode,
return true;
}

/// Helper function for building extended bit operations.
static bool buildExtendedBitOpsInst(const SPIRV::IncomingCall *Call,
unsigned Opcode,
MachineIRBuilder &MIRBuilder,
SPIRVGlobalRegistry *GR) {
const SPIRV::DemangledBuiltin *Builtin = Call->Builtin;
const auto *ST =
static_cast<const SPIRVSubtarget *>(&MIRBuilder.getMF().getSubtarget());
if ((Opcode == SPIRV::OpBitFieldInsert ||
Opcode == SPIRV::OpBitFieldSExtract ||
Opcode == SPIRV::OpBitFieldUExtract || Opcode == SPIRV::OpBitReverse) &&
!ST->canUseExtension(SPIRV::Extension::SPV_KHR_bit_instructions)) {
std::string DiagMsg = std::string(Builtin->Name) +
michalpaszkowski marked this conversation as resolved.
Show resolved Hide resolved
": the builtin requires the following SPIR-V "
"extension: SPV_KHR_bit_instructions";
report_fatal_error(DiagMsg.c_str(), false);
Copy link
Contributor

@MrSidims MrSidims Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since __spirv_BitFieldInsert and other _spirv spellings for bit instructions are untested in the backend it's hard for me to understand whether this code would work correctly. My expectations are:

  1. if OpenCL builtin calls present in LLVM IR input to the backend and SPV_KHR_bit_instructions is not enabled, then the error must be emitted;
  2. if OpenCL builtin calls present in LLVM IR input to the backend and SPV_KHR_bit_instructions is enabled, then the error must not be emitted and BitInstructions capability must be generated;
  3. if SPIR-V friendly calls are in the module (aka no OpenCL calls), then (unless there is a mechanism of distinguishing between Kernel and Shader capabilities in the backend of which I'm not yet aware of) the error must not be emitted, BitInstructions must not be generated, Shader capability must present in the module.

Will this code ensure the above behavior (it feels like scenario 3 won't work as I described) or may be I'm missing something, and my expected behavior is incorrect?

Copy link
Contributor

@MrSidims MrSidims Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for scenario 2 I don't expect Shader capability to appear (if no other shader specific instructions are used).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new tests to check these expectations. All seems to be working according to your expectations.

}

// Generate SPIRV instruction accordingly.
if (Call->isSpirvOp())
return buildOpFromWrapper(MIRBuilder, Opcode, Call, Register(0));

// Generate the instruction.
michalpaszkowski marked this conversation as resolved.
Show resolved Hide resolved
auto MIB = MIRBuilder.buildInstr(Opcode)
.addDef(Call->ReturnRegister)
.addUse(GR->getSPIRVTypeID(Call->ReturnType));
for (unsigned i = 0; i < Call->Arguments.size(); ++i)
MIB.addUse(Call->Arguments[i]);

return true;
}

static unsigned getNumComponentsForDim(SPIRV::Dim::Dim dim) {
switch (dim) {
case SPIRV::Dim::DIM_1D:
Expand Down Expand Up @@ -2041,6 +2073,17 @@ static bool generateSpecConstantInst(const SPIRV::IncomingCall *Call,
}
}

static bool generateExtendedBitOpsInst(const SPIRV::IncomingCall *Call,
MachineIRBuilder &MIRBuilder,
SPIRVGlobalRegistry *GR) {
// Lookup the instruction opcode in the TableGen records.
const SPIRV::DemangledBuiltin *Builtin = Call->Builtin;
unsigned Opcode =
SPIRV::lookupNativeBuiltin(Builtin->Name, Builtin->Set)->Opcode;

return buildExtendedBitOpsInst(Call, Opcode, MIRBuilder, GR);
}

static bool buildNDRange(const SPIRV::IncomingCall *Call,
MachineIRBuilder &MIRBuilder,
SPIRVGlobalRegistry *GR) {
Expand Down Expand Up @@ -2628,6 +2671,8 @@ std::optional<bool> lowerBuiltin(const StringRef DemangledCall,
return generateKernelClockInst(Call.get(), MIRBuilder, GR);
case SPIRV::CoopMatr:
return generateCoopMatrInst(Call.get(), MIRBuilder, GR);
case SPIRV::ExtendedBitOps:
return generateExtendedBitOpsInst(Call.get(), MIRBuilder, GR);
}
return false;
}
Expand Down
11 changes: 11 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVBuiltins.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def CastToPtr : BuiltinGroup;
def Construct : BuiltinGroup;
def CoopMatr : BuiltinGroup;
def ICarryBorrow : BuiltinGroup;
def ExtendedBitOps : BuiltinGroup;

//===----------------------------------------------------------------------===//
// Class defining a demangled builtin record. The information in the record
Expand Down Expand Up @@ -1441,6 +1442,16 @@ defm : DemangledNativeBuiltin<"__spirv_SatConvertSToU", OpenCL_std, Convert, 1,
defm : DemangledNativeBuiltin<"__spirv_SatConvertUToS", OpenCL_std, Convert, 1, 1, OpSatConvertUToS>;
defm : DemangledNativeBuiltin<"__spirv_ConvertUToPtr", OpenCL_std, Convert, 1, 1, OpConvertUToPtr>;

// cl_khr_extended_bit_ops / SPV_KHR_bit_instructions
defm : DemangledNativeBuiltin<"bitfield_insert", OpenCL_std, ExtendedBitOps, 4, 4, OpBitFieldInsert>;
defm : DemangledNativeBuiltin<"__spirv_BitFieldInsert", OpenCL_std, ExtendedBitOps, 4, 4, OpBitFieldInsert>;
Copy link
Contributor

@MrSidims MrSidims Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are SPIR-V friendly builtins tested anywhere? This PR doesn't add them and on my machine grep doesn't show any tests:

LLVM/llvm-project/llvm$ grep -rn BitFieldInsert
lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3665:static Register buildBitFieldInsert(MachineIRBuilder &B,
lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3739:    Register InsertedElt = buildBitFieldInsert(MIRBuilder, ExtractedElt,
lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:1243:  case SPIRV::OpBitFieldInsert:
lib/Target/SPIRV/SPIRVInstrInfo.td:547:def OpBitFieldInsert: Op<201, (outs ID:$res),
lib/Target/SPIRV/SPIRVInstrInfo.td:549:                  "$res = OpBitFieldInsert $ty $base $insert $offset $count">;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have been included in cl_khr_extended_bit_ops.ll.

defm : DemangledNativeBuiltin<"bitfield_extract_signed", OpenCL_std, ExtendedBitOps, 3, 3, OpBitFieldSExtract>;
defm : DemangledNativeBuiltin<"__spirv_BitFieldSExtract", OpenCL_std, ExtendedBitOps, 3, 3, OpBitFieldSExtract>;
defm : DemangledNativeBuiltin<"bitfield_extract_unsigned", OpenCL_std, ExtendedBitOps, 3, 3, OpBitFieldUExtract>;
defm : DemangledNativeBuiltin<"__spirv_BitFieldUExtract", OpenCL_std, ExtendedBitOps, 3, 3, OpBitFieldUExtract>;
defm : DemangledNativeBuiltin<"bit_reverse", OpenCL_std, ExtendedBitOps, 1, 1, OpBitReverse>;
defm : DemangledNativeBuiltin<"__spirv_BitReverse", OpenCL_std, ExtendedBitOps, 1, 1, OpBitReverse>;

// cl_intel_bfloat16_conversions / SPV_INTEL_bfloat16_conversion
// Multiclass used to define at the same time both a demangled builtin records
// and a corresponding convert builtin records.
Expand Down
Loading
Loading