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

steffenlarsen · 2022-09-16T10:50:15Z

This commit adds a new module-level metadata node !intel_sycl_aspects which contains metadata pairs of the enum element names and integral values of the SYCL aspects enum, identified by the [[sycl_detail::sycl_type(aspect)]] attribute.
This commit also makes the SYCLPropagateAspectsUsage pass read !intel_sycl_aspects and use this information to determine the value of the fp64 aspect instead of relying on it being synchronized with the SYCL implementation headers.

This commit adds a new module-level metadata node !intel_sycl_aspects which contains metadata pairs of the enum element names and integral values of the SYCL aspects enum, identified by the [[__sycl_detail__::sycl_type(aspect)]] attribute. This commit also makes the SYCLPropagateAspectsUsage pass read !intel_sycl_aspects and use this information to determine the value of the fp64 aspect instead of relying on it being synchronized with the SYCL implementation headers. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

clang/test/CodeGenSYCL/aspect_enum.cpp

bader · 2022-09-17T08:46:26Z

clang/lib/CodeGen/CodeGenModule.cpp

+    // Emit metadata for all aspects defined in the aspects enum.
+    if (AspectsEnumDecl) {
+      llvm::NamedMDNode *AspectEnumValsMD =
+          TheModule.getOrInsertNamedMetadata("intel_sycl_aspects");


I suggest we drop "intel_" prefix. The attribute value is not intel specific and it should be easier to upstream w/o the prefix.

The same could be said for every other LLVM attribute described in this design (e.g. intel_declared_aspects, intel_used_aspects). We have existing LLVM attributes named this way too (e.g. intel_reqd_sub_group_size). If we think we want to rename SYCL-related LLVM attributes to make them easier to upstream, we should make some sort of global decision and rename them all.

intel_reqd_sub_group_size was introduced for Intel specific extension, whereas aspect is already SYCL core feature.

The [[sycl::reqd_sub_group_size()]] C++ attribute is also a core SYCL feature now.

That's right, but what "intel_" in "intel_sycl_aspects" refers to? Is this the name of some type in the extension? I assumed that it refers to "Intel specific implementation" and this will be difficult to upstream.
I think we agree that things should be named after the spec entities, rather than implementation names like dpcpp, intel, etc. if we aim to commit this code to LLVM project.

I'm not opposed to renaming these LLVM IR attributes, I just think they should all be named consistently. If we want to remove the "intel" prefix, maybe the new names could be like:

intel_types_that_use_aspects -> sycl_types_that_use_aspects

intel_declared_aspects -> sycl_declared_aspects

intel_used_aspects -> sycl_used_aspects

intel_fixed_targets -> sycl_fixed_targets

intel_sycl_aspects -> sycl_aspects

In addition, we should identify other sycl-related LLVM IR attributes that start with "intel" and rename them to start with "sycl". This PR could rename the attributes defined in the OptionalDeviceFeratures design, and we could create a tracker to capture the work required to rename any other attributes.

Alternatively, we could keep the names as "intel" in this PR and create a new PR that renames everything to "sycl" prefix.

Yes, please, update other LLVM IR attributes (if it's possible to do w/o ABI breaking) too. This PR mentions only one attribute and I was not aware that there other similar attributes.

I am happy to go rename the metadata nodes related to OptionalDeviceFeratures, but I would prefer to do it in a separate PR to avoid polluting this one. That said, since intel_sycl_aspects is introduced with this I will make the renaming of it as part of this PR.

gmlueck · 2022-09-19T12:28:57Z

sycl/doc/design/OptionalDeviceFeatures.md

+!1 = !{!"cpu", i32 1}
+!2 = !{!"gpu", i32 2}
+...
+```


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"

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.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

asudarsa · 2022-09-20T14:36:50Z

llvm/lib/SYCLLowerIR/SYCLPropagateAspectsUsage.cpp

+  if (AspectValues.empty()) {
+    assert(TypesWithAspects.empty() &&
+           "sycl_aspects metadata is missing but "
+           "intel_types_that_use_aspects is present.");


I suppose this needs renaming at some point to sycl_?

Good eye! Yes, I might as well do that renaming now.

asudarsa · 2022-09-20T14:46:47Z

llvm/lib/SYCLLowerIR/SYCLPropagateAspectsUsage.cpp

@@ -333,7 +362,19 @@ buildFunctionsToAspectsMap(Module &M, TypeToAspectsMapTy &TypesWithAspects) {
 PreservedAnalyses
 SYCLPropagateAspectsUsagePass::run(Module &M, ModuleAnalysisManager &MAM) {
  TypeToAspectsMapTy TypesWithAspects = getTypesThatUseAspectsFromMetadata(M);
-  propagateAspectsToOtherTypesInModule(M, TypesWithAspects);
+  AspectValueToNameMapTy AspectValues = getSYCLAspectsFromMetadata(M);


Why is the function named 'getSYCLAspects' and not just 'getAspects'?

It made sense when the metadata was named intel_sycl_aspects, but now I think I agree that getAspects looks better. It has been renamed.

asudarsa · 2022-09-20T14:51:55Z

llvm/lib/SYCLLowerIR/SYCLPropagateAspectsUsage.cpp

@@ -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());


Do we need to check if DoubleTy could be NULL?

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.

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.

asudarsa

SYCLLowerIR pass looks good to me. Just a few nitpicks. Nothing blocking. Thanks

premanandrao · 2022-09-20T14:58:30Z

clang/lib/CodeGen/CodeGenModule.cpp

+  AspectEnumValMD.push_back(llvm::MDString::get(Ctx, ECD->getName()));
+  AspectEnumValMD.push_back(
+      llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
+          llvm::Type::getInt32Ty(Ctx), ECD->getInitVal().getZExtValue())));


Should this be getSExtValue()?

I am not sure it matters for this specific case, but better keep it consistent! It has been changed, thanks!

llvm/lib/SYCLLowerIR/SYCLPropagateAspectsUsage.cpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

clang/test/CodeGenSYCL/multiple_aspect_enum.cpp

asudarsa

LGTM. Thanks

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

…_info

steffenlarsen requested review from bader, AlexeySachkov and elizabethandrews September 16, 2022 10:50

steffenlarsen requested review from a team as code owners September 16, 2022 10:50

elizabethandrews reviewed Sep 16, 2022

View reviewed changes

clang/test/CodeGenSYCL/aspect_enum.cpp Outdated Show resolved Hide resolved

bader reviewed Sep 18, 2022

View reviewed changes

gmlueck reviewed Sep 19, 2022

View reviewed changes

Address review comments

df9531f

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen mentioned this pull request Sep 20, 2022

[SYCL] Rename optional device feature metadata #6822

Merged

gmlueck previously approved these changes Sep 20, 2022

View reviewed changes

asudarsa reviewed Sep 20, 2022

View reviewed changes

asudarsa previously approved these changes Sep 20, 2022

View reviewed changes

premanandrao reviewed Sep 20, 2022

View reviewed changes

Fznamznon previously approved these changes Sep 20, 2022

View reviewed changes

steffenlarsen added 2 commits September 20, 2022 11:03

Address more review comments

6622dcc

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Rename ahead of renamings

0c6d8d7

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen dismissed stale reviews from Fznamznon, asudarsa, and gmlueck via 6622dcc September 20, 2022 18:06

Fznamznon reviewed Sep 20, 2022

View reviewed changes

clang/test/CodeGenSYCL/multiple_aspect_enum.cpp Outdated Show resolved Hide resolved

asudarsa approved these changes Sep 20, 2022

View reviewed changes

Remove -fsycl-only from multiple_aspect_enum test

d5a30c2

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Fznamznon approved these changes Sep 21, 2022

View reviewed changes

smanna12 approved these changes Sep 21, 2022

View reviewed changes

premanandrao approved these changes Sep 21, 2022

View reviewed changes

gmlueck approved these changes Sep 23, 2022

View reviewed changes

Merge remote-tracking branch 'intel/sycl' into steffen/aspect_enum_ir…

8e74110

…_info

AlexeySachkov approved these changes Sep 23, 2022

View reviewed changes

steffenlarsen merged commit 4d0df22 into intel:sycl Sep 23, 2022

AlexeySachkov mentioned this pull request Oct 20, 2022

[SYCL] Propagate sycl_fixed_targets metadata #7115

Merged

elizabethandrews mentioned this pull request Dec 9, 2022

[SYCL] Rename intel_reqd_sub_group_size attribute metadata #7713

Closed

AlexeySachkov mentioned this pull request Dec 14, 2022

SYCLPropagateAspecUsage pass: compiler and runtime must not be tied to each other #5892

Closed

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

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

Uh oh!

Conversation

steffenlarsen commented Sep 16, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader Sep 19, 2022 •

edited

Loading

asudarsa Sep 20, 2022 •

edited

Loading