-
Notifications
You must be signed in to change notification settings - Fork 779
[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
Changes from all commits
61d6ea7
df9531f
6622dcc
0c6d8d7
d5a30c2
8e74110
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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} |
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 | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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} | ||
... | ||
``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not very clear from this design document how the Did you consider as an alternate solution to have the CFE include one more entry in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Absolutely! I'll add a note.
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 | ||
|
@@ -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 | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 benullptr
.We definitely don't need to insert a runtime
if
here and even assertion would excessive, I think.