Skip to content

[SYCL] Add aspect enum value information to LLVM IR metadata #6804

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.

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

Merged
merged 6 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
30 changes: 30 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,17 @@ static llvm::MDNode *getAspectsMD(ASTContext &ASTContext,
return llvm::MDNode::get(Ctx, AspectsMD);
}

static llvm::MDNode *getAspectEnumValueMD(ASTContext &ASTContext,
llvm::LLVMContext &Ctx,
const EnumConstantDecl *ECD) {
SmallVector<llvm::Metadata *, 2> AspectEnumValMD;
AspectEnumValMD.push_back(llvm::MDString::get(Ctx, ECD->getName()));
AspectEnumValMD.push_back(
llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
llvm::Type::getInt32Ty(Ctx), ECD->getInitVal().getSExtValue())));
return llvm::MDNode::get(Ctx, AspectEnumValMD);
}

void CodeGenModule::Release() {
Module *Primary = getContext().getModuleForCodeGen();
if (CXX20ModuleInits && Primary && !Primary->isHeaderLikeModule())
Expand Down Expand Up @@ -924,6 +935,15 @@ void CodeGenModule::Release() {
RD->getAttr<SYCLUsesAspectsAttr>()));
}
}

// Emit metadata for all aspects defined in the aspects enum.
if (AspectsEnumDecl) {
llvm::NamedMDNode *AspectEnumValsMD =
TheModule.getOrInsertNamedMetadata("sycl_aspects");
for (const EnumConstantDecl *ECD : AspectsEnumDecl->enumerators())
AspectEnumValsMD->addOperand(
getAspectEnumValueMD(Context, TheModule.getContext(), ECD));
}
}

// HLSL related end of code gen work items.
Expand Down Expand Up @@ -5063,6 +5083,16 @@ void CodeGenModule::maybeSetTrivialComdat(const Decl &D,
GO.setComdat(TheModule.getOrInsertComdat(GO.getName()));
}

void CodeGenModule::setAspectsEnumDecl(const EnumDecl *ED) {
if (AspectsEnumDecl && AspectsEnumDecl != ED) {
// Conflicting definitions of the aspect enum are not allowed.
Error(ED->getLocation(), "redefinition of aspect enum");
getDiags().Report(AspectsEnumDecl->getLocation(),
diag::note_previous_definition);
}
AspectsEnumDecl = ED;
}

void CodeGenModule::generateIntelFPGAAnnotation(
const Decl *D, llvm::SmallString<256> &AnnotStr) {
llvm::raw_svector_ostream Out(AnnotStr);
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,7 @@ class CodeGenModule : public CodeGenTypeCache {
llvm::DenseMap<const llvm::Constant *, llvm::GlobalVariable *> RTTIProxyMap;

llvm::DenseMap<StringRef, const RecordDecl *> TypesWithAspects;
const EnumDecl *AspectsEnumDecl = nullptr;

public:
CodeGenModule(ASTContext &C, IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
Expand Down Expand Up @@ -1098,6 +1099,8 @@ class CodeGenModule : public CodeGenTypeCache {
TypesWithAspects[TypeName] = RD;
}

void setAspectsEnumDecl(const EnumDecl *ED);

void generateIntelFPGAAnnotation(const Decl *D,
llvm::SmallString<256> &AnnotStr);
void addGlobalIntelFPGAAnnotation(const VarDecl *VD, llvm::GlobalValue *GV);
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/CodeGen/CodeGenTypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,10 @@ void CodeGenTypes::UpdateCompletedType(const TagDecl *TD) {
if (!ConvertType(ED->getIntegerType())->isIntegerTy(32))
TypeCache.clear();
}
// If this is the SYCL aspect enum it is saved for later processing.
if (const auto *Attr = ED->getAttr<SYCLTypeAttr>())
if (Attr->getType() == SYCLTypeAttr::SYCLType::aspect)
CGM.setAspectsEnumDecl(ED);
// If necessary, provide the full definition of a type only used with a
// declaration so far.
if (CGDebugInfo *DI = CGM.getModuleDebugInfo())
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/Inputs/sycl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ enum class address_space : int {
} // namespace access

// Dummy aspect enum with limited enumerators
enum class __SYCL_TYPE(aspect) aspect {
enum class __SYCL_TYPE(aspect) aspect { // #AspectEnum
host = 0,
cpu = 1,
gpu = 2,
Expand Down
13 changes: 13 additions & 0 deletions clang/test/CodeGenSYCL/aspect_enum.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: %clang_cc1 -internal-isystem %S/Inputs -fsycl-is-device -triple spir64-unknown-unknown -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s

// Tests for IR of [[__sycl_detail__::sycl_type(aspect)]] enum.
#include "sycl.hpp"

// CHECK: !sycl_aspects = !{![[HOST:[0-9]+]], ![[CPU:[0-9]+]], ![[GPU:[0-9]+]], ![[ACC:[0-9]+]], ![[CUSTOM:[0-9]+]], ![[FP16:[0-9]+]], ![[FP64:[0-9]+]]}
// CHECK: ![[HOST]] = !{!"host", i32 0}
// CHECK: ![[CPU]] = !{!"cpu", i32 1}
// CHECK: ![[GPU]] = !{!"gpu", i32 2}
// CHECK: ![[ACC]] = !{!"accelerator", i32 3}
// CHECK: ![[CUSTOM]] = !{!"custom", i32 4}
// CHECK: ![[FP16]] = !{!"fp16", i32 5}
// CHECK: ![[FP64]] = !{!"fp64", i32 6}
12 changes: 12 additions & 0 deletions clang/test/CodeGenSYCL/multiple_aspect_enum.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %clang_cc1 -internal-isystem %S/Inputs -fsycl-is-device -triple spir64-unknown-unknown -verify -emit-llvm-only %s

// Tests for error diagnostics when multiple definitions of
// [[__sycl_detail__::sycl_type(aspect)]] enums are present.
#include "sycl.hpp"

// expected-note@#AspectEnum{{previous definition is here}}

// expected-error@+1{{redefinition of aspect enum}}
enum class [[__sycl_detail__::sycl_type(aspect)]] aspect_redef {
imposter_value = 3
};
61 changes: 51 additions & 10 deletions llvm/lib/SYCLLowerIR/SYCLPropagateAspectsUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,35 @@ TypeToAspectsMapTy getTypesThatUseAspectsFromMetadata(const Module &M) {
return Result;
}

using AspectValueToNameMapTy = SmallMapVector<StringRef, int, 32>;

/// Retrieves from metadata (sycl_aspects) the mapping between SYCL aspect names
/// and their integral values.
AspectValueToNameMapTy getAspectsFromMetadata(const Module &M) {
const NamedMDNode *Node = M.getNamedMetadata("sycl_aspects");
AspectValueToNameMapTy Result;
if (!Node)
return Result;

for (const auto OperandIt : Node->operands()) {
const MDNode &N = *OperandIt;
assert(N.getNumOperands() == 2 &&
"Each operand of sycl_aspects must be a pair.");

// The aspect's name is the first operand.
const auto *AspectName = cast<MDString>(N.getOperand(0));

// The aspect's integral value is the second operand.
const auto *AspectCAM = cast<ConstantAsMetadata>(N.getOperand(1));
const Constant *AspectC = AspectCAM->getValue();

Result[AspectName->getString()] =
cast<ConstantInt>(AspectC)->getSExtValue();
}

return Result;
}

using TypesEdgesTy =
std::unordered_map<const Type *, std::vector<const Type *>>;

Expand Down Expand Up @@ -107,6 +136,7 @@ void propagateAspectsThroughTypes(const TypesEdgesTy &Edges, const Type *Start,
/// another type TT, which in turn uses the aspect A.
/// @TypesWithAspects argument consist of known types with aspects
/// from metadata information.
/// @AspectValues argument consist of known aspect values and their names.
///
/// The algorithm is the following:
/// 1) Make a list of all structure types from module @M. The list also
Expand All @@ -121,18 +151,17 @@ void propagateAspectsThroughTypes(const TypesEdgesTy &Edges, const Type *Start,
/// Time complexity: O((V + E) * T) where T is the number of input types
/// containing aspects.
void propagateAspectsToOtherTypesInModule(
const Module &M, TypeToAspectsMapTy &TypesWithAspects) {
const Module &M, TypeToAspectsMapTy &TypesWithAspects,
AspectValueToNameMapTy &AspectValues) {
std::unordered_set<const Type *> TypesToProcess;
const Type *DoubleTy = Type::getDoubleTy(M.getContext());
Copy link
Contributor

@asudarsa asudarsa Sep 20, 2022

Choose a reason for hiding this comment

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

Do we need to check if DoubleTy could be NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so. To my knowledge the context should be able to supply it in all cases we care about, but @AlexeySachkov may know better.

Copy link
Contributor

Choose a reason for hiding this comment

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

To my knowledge the context should be able to supply it in all cases we care about, but @AlexeySachkov may know better.

I don't see any guarantees in the doxygen documentation, but looking at the source code we simply return an address of a LLVMContextImpl field, which can't be nullptr.

We definitely don't need to insert a runtime if here and even assertion would excessive, I think.


// 6 is taken from sycl/include/CL/sycl/aspects.hpp
// Note: that magic number must strictly correspond to the one assigned to
// 'fp64' value of 'aspect' enum.
// FIXME: we should develop some kind of mechanism which will allow us to
// avoid hardcoding this number here and having a build dependency between
// the compiler and the runtime. See intel/llvm#5892
static constexpr int AspectFP64 = 6;
TypesWithAspects[DoubleTy].insert(AspectFP64);
// Find the value of the fp64 aspect from the aspect values map and register
// it as a special-case type with aspect for double.
auto FP64AspectIt = AspectValues.find("fp64");
assert(FP64AspectIt != AspectValues.end() &&
"fp64 aspect was not found in the aspect values.");
TypesWithAspects[DoubleTy].insert(FP64AspectIt->second);

TypesToProcess.insert(DoubleTy);
for (const Type *T : M.getIdentifiedStructTypes())
Expand Down Expand Up @@ -339,7 +368,19 @@ buildFunctionsToAspectsMap(Module &M, TypeToAspectsMapTy &TypesWithAspects) {
PreservedAnalyses
SYCLPropagateAspectsUsagePass::run(Module &M, ModuleAnalysisManager &MAM) {
TypeToAspectsMapTy TypesWithAspects = getTypesThatUseAspectsFromMetadata(M);
propagateAspectsToOtherTypesInModule(M, TypesWithAspects);
AspectValueToNameMapTy AspectValues = getAspectsFromMetadata(M);

// If there is no metadata for aspect values the source code must not have
// included the SYCL headers. In that case there should also not be any types
// that use aspects, so we can skip this pass.
if (AspectValues.empty()) {
assert(TypesWithAspects.empty() &&
"sycl_aspects metadata is missing but "
"sycl_types_that_use_aspects is present.");
return PreservedAnalyses::all();
}

propagateAspectsToOtherTypesInModule(M, TypesWithAspects, AspectValues);

FunctionToAspectsMapTy FunctionToAspects =
buildFunctionsToAspectsMap(M, TypesWithAspects);
Expand Down
3 changes: 3 additions & 0 deletions llvm/test/SYCLLowerIR/PropagateAspectsUsage/call-graph-1.ll
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ define spir_func void @func3() {
!0 = !{!"Optional.A", i32 1}
!1 = !{!"Optional.B", i32 2}

!sycl_aspects = !{!2}
!2 = !{!"fp64", i32 6}

; CHECK: ![[#ID1]] = !{i32 1}
; CHECK: ![[#ID2]] = !{i32 1, i32 2}
; CHECK: ![[#ID3]] = !{i32 2}
3 changes: 3 additions & 0 deletions llvm/test/SYCLLowerIR/PropagateAspectsUsage/call-graph-2.ll
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ define spir_func void @func4() {
!0 = !{!"Optional.A", i32 1}
!1 = !{!"Optional.B", i32 2}

!sycl_aspects = !{!2}
!2 = !{!"fp64", i32 6}

; CHECK: ![[#ID1]] = !{i32 1, i32 2}
; CHECK: ![[#ID2]] = !{i32 1}
; CHECK: ![[#ID3]] = !{i32 2}
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@

%F2.does.not.contain.optional = type { %B.core, %C.core*, %D2.does.not.contain.optional* }

; CHECK: spir_kernel void @kernelD1.uses.optional() !sycl_used_aspects !1 {
; CHECK: spir_kernel void @kernelD1.uses.optional() !sycl_used_aspects ![[MDID:[0-9]+]] {
define spir_kernel void @kernelD1.uses.optional() {
%tmp = alloca %D1.contains.optional
ret void
}

; CHECK: spir_func void @funcD1.uses.optional() !sycl_used_aspects !1 {
; CHECK: spir_func void @funcD1.uses.optional() !sycl_used_aspects ![[MDID]] {
define spir_func void @funcD1.uses.optional() {
%tmp = alloca %D1.contains.optional
ret void
Expand All @@ -46,13 +46,13 @@ define spir_func void @funcD2.does.not.use.optional() {
ret void
}

; CHECK: spir_kernel void @kernelE.uses.optional() !sycl_used_aspects !1 {
; CHECK: spir_kernel void @kernelE.uses.optional() !sycl_used_aspects ![[MDID]] {
define spir_kernel void @kernelE.uses.optional() {
%tmp = alloca %E.contains.optional
ret void
}

; CHECK: spir_func void @funcE.uses.optional() !sycl_used_aspects !1 {
; CHECK: spir_func void @funcE.uses.optional() !sycl_used_aspects ![[MDID]] {
define spir_func void @funcE.uses.optional() {
%tmp = alloca %E.contains.optional
ret void
Expand Down Expand Up @@ -82,25 +82,28 @@ define spir_func void @funcF2.does.not.use.optional() {
ret void
}

; CHECK: spir_func %A.optional @funcA.returns.optional() !sycl_used_aspects !1 {
; CHECK: spir_func %A.optional @funcA.returns.optional() !sycl_used_aspects ![[MDID]] {
define spir_func %A.optional @funcA.returns.optional() {
%tmp = alloca %A.optional
%ret = load %A.optional, %A.optional* %tmp
ret %A.optional %ret
}

; CHECK: spir_func void @funcA.uses.array.of.optional() !sycl_used_aspects !1 {
; CHECK: spir_func void @funcA.uses.array.of.optional() !sycl_used_aspects ![[MDID]] {
define spir_func void @funcA.uses.array.of.optional() {
%tmp = alloca [4 x %A.optional]
ret void
}

; CHECK: spir_func void @funcA.assepts.optional(%A.optional %0) !sycl_used_aspects !1 {
; CHECK: spir_func void @funcA.assepts.optional(%A.optional %0) !sycl_used_aspects ![[MDID]] {
define spir_func void @funcA.assepts.optional(%A.optional %0) {
ret void
}

!sycl_types_that_use_aspects = !{!0}
!0 = !{!"A.optional", i32 1}

; CHECK: !1 = !{i32 1}
!sycl_aspects = !{!1}
!1 = !{!"fp64", i32 6}

; CHECK: ![[MDID]] = !{i32 1}
15 changes: 9 additions & 6 deletions llvm/test/SYCLLowerIR/PropagateAspectsUsage/double.ll
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,37 @@

%composite = type { double }

; CHECK: spir_kernel void @kernel() !sycl_used_aspects !0 {
; CHECK: spir_kernel void @kernel() !sycl_used_aspects ![[MDID:[0-9]+]] {
define spir_kernel void @kernel() {
call spir_func void @func()
ret void
}

; CHECK: spir_func void @func() !sycl_used_aspects !0 {
; CHECK: spir_func void @func() !sycl_used_aspects ![[MDID]] {
define spir_func void @func() {
%tmp = alloca double
ret void
}

; CHECK: spir_func void @func.array() !sycl_used_aspects !0 {
; CHECK: spir_func void @func.array() !sycl_used_aspects ![[MDID]] {
define spir_func void @func.array() {
%tmp = alloca [4 x double]
ret void
}

; CHECK: spir_func void @func.vector() !sycl_used_aspects !0 {
; CHECK: spir_func void @func.vector() !sycl_used_aspects ![[MDID]] {
define spir_func void @func.vector() {
%tmp = alloca <4 x double>
ret void
}

; CHECK: spir_func void @func.composite() !sycl_used_aspects !0 {
; CHECK: spir_func void @func.composite() !sycl_used_aspects ![[MDID]] {
define spir_func void @func.composite() {
%tmp = alloca %composite
ret void
}

; CHECK: !0 = !{i32 6}
!sycl_aspects = !{!0}
!0 = !{!"fp64", i32 6}

; CHECK: ![[MDID]] = !{i32 6}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ define spir_kernel void @kernel() {
!2 = !{!"C", i32 2}
!3 = !{!"D", i32 3, i32 4}

!sycl_aspects = !{!4}
!4 = !{!"fp64", i32 6}

; CHECK: ![[#ID0]] = !{i32 0}
; CHECK: ![[#ID1]] = !{i32 1, i32 0}
; CHECK: ![[#ID2]] = !{i32 2, i32 1, i32 0}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,6 @@ define weak dso_local spir_func void @func() {

!sycl_types_that_use_aspects = !{!0}
!0 = !{!"MyStruct", i32 1}

!sycl_aspects = !{!2}
!2 = !{!"fp64", i32 6}
24 changes: 24 additions & 0 deletions sycl/doc/design/OptionalDeviceFeatures.md
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,24 @@ allow metadata to be attached directly to types. This representation works
around that limitation by creating global named metadata that references the
type's name.

To synchronize the integral values of given aspects between the SYCL headers and
the compiler, the `!sycl_aspects` metadata is added to the module, based on the
values defined in the enum. Inside this metadata node, each value of the aspect
enum is represented by another metadata node with two operands; the name of the
value and the corresponding integral value. An example of this is:

```
!sycl_aspects = !{!0, !1, !2, ...}
!0 = !{!"host", i32 0}
!1 = !{!"cpu", i32 1}
!2 = !{!"gpu", i32 2}
...
```
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not very clear from this design document how the intel_sycl_aspects metadata is used. I think the reason is to provide the enum value for aspect::fp64 to the new LLVM IR pass, correct? If that is the case, can you add a note to the description of that pass saying that it uses this metadata for that purpose?

Did you consider as an alternate solution to have the CFE include one more entry in intel_types_that_use_aspects? For example:

!intel_types_that_use_aspects = !{!0, ...}
!0 = !{!"double", i32 8}  # The value 8 is "aspect::fp64"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not very clear from this design document how the intel_sycl_aspects metadata is used. I think the reason is to provide the enum value for aspect::fp64 to the new LLVM IR pass, correct? If that is the case, can you add a note to the description of that pass saying that it uses this metadata for that purpose?

Absolutely! I'll add a note.

Did you consider as an alternate solution to have the CFE include one more entry in intel_types_that_use_aspects? For example:

!intel_types_that_use_aspects = !{!0, ...}
!0 = !{!"double", i32 8}  # The value 8 is "aspect::fp64"

I did consider this, but having all of them present has the added benefit of allowing us to map values to aspects when diagnostics are added in a follow-up patch.


**NOTE**: The `!sycl_aspects` metadata is both used by the compiler to identify
the aspect values of implicit aspect requirements, such as `aspect::fp64` from
the use of `double`, and to offer better diagnostic messages.

We also introduce three metadata that can be attached to a function definition:

* The `!sycl_declared_aspects` metadata is used for functions that are
Expand Down Expand Up @@ -483,6 +501,12 @@ to the following rules:
an `!sycl_declared_aspects` metadata to the function's definition listing
the aspects from that attribute.

* If a completed enum is decorated with `[[sycl_detail::sycl_type(aspect)]]` the
front-end adds an `!sycl_aspects` metadata to the module containing one
metadata node for each value in the enum. If there are multiple enum
definitions with the `[[sycl_detail::sycl_type(aspect)]]` attribute a
diagnostic is issued.


### New LLVM IR pass to propagate aspect usage

Expand Down