Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
19 changes: 9 additions & 10 deletions lib/SPIRV/SPIRVReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,19 +200,19 @@ static void addRuntimeAlignedMetadata(
LLVMContext *Context, SPIRVFunction *BF, llvm::Function *Fn,
std::function<Metadata *(SPIRVFunctionParameter *)> ForeachFnArg) {
std::vector<Metadata *> ValueVec;
bool DecorationFound = false;
bool RuntimeAlignedFound = false;
BF->foreachArgument([&](SPIRVFunctionParameter *Arg) {
if (Arg->getType()->isTypePointer() &&
if (Arg->hasAttr(FunctionParameterAttributeRuntimeAlignedINTEL) ||
Arg->hasDecorate(internal::DecorationRuntimeAlignedINTEL)) {
Copy link
Contributor

@broxigarchen broxigarchen Mar 15, 2023

Choose a reason for hiding this comment

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

we finally will remove this line
"Arg->hasDecorate(internal::DecorationRuntimeAlignedINTEL)"
right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I believe that is the intention - but only after consumers are ready to work with the correct implementation.

DecorationFound = true;
RuntimeAlignedFound = true;
ValueVec.push_back(ForeachFnArg(Arg));
Copy link
Contributor

Choose a reason for hiding this comment

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

ForeachFnArg seems unnecessary here. We can just create a ConstantInt with 1 and push it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

} else {
llvm::Metadata *DefaultNode = ConstantAsMetadata::get(
Copy link
Contributor

Choose a reason for hiding this comment

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

One nit: Instead of creating DefaultNode for each Argument, we can create a single static global DefaultNode and push it when required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Guess no need to have it as a global, moved it out of lambda to have it initialized only once in the function.

ConstantInt::get(Type::getInt1Ty(*Context), 0));
ValueVec.push_back(DefaultNode);
}
});
if (DecorationFound)
if (RuntimeAlignedFound)
Fn->setMetadata("kernel_arg_runtime_aligned",
MDNode::get(*Context, ValueVec));
}
Expand Down Expand Up @@ -2983,6 +2983,10 @@ void SPIRVToLLVM::transFunctionAttrs(SPIRVFunction *BF, Function *F) {
setName(&(*I), BA);
AttributeMask IllegalAttrs = AttributeFuncs::typeIncompatible(I->getType());
BA->foreachAttr([&](SPIRVFuncParamAttrKind Kind) {
// Skip this function parameter attribute as it will translated among
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, why wasn't it skipped before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it wasn't a function parameter attribute (since it was implemented incorrectly)

// OpenCL metadata
if (Kind == FunctionParameterAttributeRuntimeAlignedINTEL)
return;
Attribute::AttrKind LLVMKind = SPIRSPIRVFuncParamAttrMap::rmap(Kind);
if (IllegalAttrs.contains(LLVMKind))
return;
Expand Down Expand Up @@ -4465,13 +4469,8 @@ bool SPIRVToLLVM::transOCLMetadata(SPIRVFunction *BF) {
});
// Generate metadata for kernel_arg_runtime_aligned
addRuntimeAlignedMetadata(Context, BF, F, [=](SPIRVFunctionParameter *Arg) {
auto Literals =
Arg->getDecorationLiterals(internal::DecorationRuntimeAlignedINTEL);
assert(Literals.size() == 1 &&
"RuntimeAlignedINTEL decoration shall have 1 ID literal");

return ConstantAsMetadata::get(
ConstantInt::get(Type::getInt1Ty(*Context), Literals[0]));
ConstantInt::get(Type::getInt1Ty(*Context), 1));
});
// Generate metadata for spirv.ParameterDecorations
addKernelArgumentMetadata(Context, SPIRV_MD_PARAMETER_DECORATIONS, BF, F,
Expand Down
15 changes: 11 additions & 4 deletions lib/SPIRV/SPIRVWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -951,12 +951,19 @@ SPIRVFunction *LLVMToSPIRVBase::transFunctionDecl(Function *F) {
// Order of integer numbers in MD node follows the order of function
// parameters on which we shall attach the appropriate decoration. Add
// decoration only if MD value is 1.
int LocID = 0;
int IsRuntimeAligned = 0;
if (!isa<MDString>(RuntimeAligned->getOperand(ArgNo)) &&
!isa<MDNode>(RuntimeAligned->getOperand(ArgNo)))
LocID = getMDOperandAsInt(RuntimeAligned, ArgNo);
if (LocID == 1)
BA->addDecorate(internal::DecorationRuntimeAlignedINTEL, LocID);
IsRuntimeAligned = getMDOperandAsInt(RuntimeAligned, ArgNo);
if (IsRuntimeAligned == 1) {
// TODO: to replace non-conformant to the spec decoration generation
// with:
// BM->addExtension(ExtensionID::SPV_INTEL_runtime_aligned);
// BM->addCapability(CapabilityRuntimeAlignedAttributeINTEL);
// BA->addAttr(FunctionParameterAttributeRuntimeAlignedINTEL);
BA->addDecorate(internal::DecorationRuntimeAlignedINTEL,
IsRuntimeAligned);
}
}
}
if (Attrs.hasRetAttr(Attribute::ZExt))
Expand Down
1 change: 1 addition & 0 deletions lib/SPIRV/libSPIRV/SPIRVIsValidEnum.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ inline bool isValid(spv::FunctionParameterAttribute V) {
case FunctionParameterAttributeNoCapture:
case FunctionParameterAttributeNoWrite:
case FunctionParameterAttributeNoReadWrite:
case FunctionParameterAttributeRuntimeAlignedINTEL:
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this change will be useful when SPIRVWriter.cpp is updated.

return true;
default:
return false;
Expand Down
68 changes: 68 additions & 0 deletions test/extensions/INTEL/SPV_INTEL_runtime_aligned/RuntimeAligned.spt
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
; RUN: llvm-spirv -spirv-text -r %s -o %t.bc
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR. Going forward, it will be great if we can introduce such .spt tests by generating them using the spirv-dis tool which generates a more readable file.

Thanks

; RUN: llvm-dis < %t.bc | FileCheck %s --check-prefix=CHECK-LLVM

; CHECK-LLVM: define spir_kernel void @test{{.*}} !kernel_arg_runtime_aligned ![[RTALIGN_MD:[0-9]+]] {{.*}}
; CHECK-LLVM: ![[RTALIGN_MD]] = !{i1 true, i1 false, i1 true, i1 false, i1 false}

119734787 65536 393230 22 0
2 Capability Addresses
2 Capability Linkage
2 Capability Kernel
2 Capability Int8
2 Capability RuntimeAlignedAttributeINTEL
8 Extension "SPV_INTEL_runtime_aligned"
5 ExtInstImport 1 "OpenCL.std"
3 MemoryModel 2 2
5 EntryPoint 6 14 "test"
3 Source 0 0
4 Name 7 "test"
3 Name 8 "a"
3 Name 9 "b"
3 Name 10 "c"
3 Name 11 "d"
3 Name 12 "e"
4 Name 13 "entry"
3 Name 15 "a"
3 Name 16 "b"
3 Name 17 "c"
3 Name 18 "d"
3 Name 19 "e"

6 Decorate 7 LinkageAttributes "test" Export
4 Decorate 8 FuncParamAttr 5940
4 Decorate 10 FuncParamAttr 5940
4 Decorate 15 FuncParamAttr 5940
4 Decorate 17 FuncParamAttr 5940
4 TypeInt 3 8 0
4 TypeInt 5 32 0
2 TypeVoid 2
4 TypePointer 4 5 3
8 TypeFunction 6 2 4 4 4 5 5



5 Function 2 7 0 6
3 FunctionParameter 4 8
3 FunctionParameter 4 9
3 FunctionParameter 4 10
3 FunctionParameter 5 11
3 FunctionParameter 5 12

2 Label 13
1 Return

1 FunctionEnd

5 Function 2 14 0 6
3 FunctionParameter 4 15
3 FunctionParameter 4 16
3 FunctionParameter 4 17
3 FunctionParameter 5 18
3 FunctionParameter 5 19

2 Label 20
9 FunctionCall 2 21 7 15 16 17 18 19
1 Return

1 FunctionEnd