-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Generate property for default specialization constants values #4845
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
Conversation
Signed-off-by: mdimakov <[email protected]>
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.
The change looks good.
Though it should have a test.
I can't think of a good unit-test for RT here.
An E2E in intel/llvm-test-suite seems to be a good choice.
e2504ee
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.
@maximdimakov, there seems to be a regression of spec_constants_bool
ESIMD test, could you please take a look?
@@ -235,7 +235,7 @@ attributes #3 = { nounwind } | |||
; CHECK: !sycl.specialization-constants = !{![[#ID0:]], ![[#ID1:]], ![[#ID2:]], ![[#ID3:]], ![[#ID_COMPOS3:]], ![[#ID4:]], ![[#ID5:]] | |||
; | |||
; CHECK-DEF: !sycl.specialization-constants-default-values = !{![[#ID4:]], ![[#ID5:]], ![[#ID6:]], ![[#ID7:]], ![[#ID_COMPOS3_DEFAULT:]], ![[#ID8:]], ![[#ID9:]] | |||
; CHECK-RT-NOT: !sycl.specialization-constants-default-values | |||
; CHECK-RT: !sycl.specialization-constants-default-values |
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.
Ideally, we should be unifying CHECK-DEF
and CHECK-RT
for that metadata into a common CHECK
, i.e. we should check content of the metadata in both modes.
However, this test is quite complicated and messy already, I plan to refactor those LIT tests to make them cleaner and simpler. Therefore, I'm ok to accept this test as-is.
The problem with spec_const_bool.cpp in precommit happened due to XFAIL line in the test. Fix for it is in the : intel/llvm-test-suite#556 |
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.
LGTM
Generate property not only for native mode helps to DPCPP RT get correct default values of specialization constants.
Signed-off-by: mdimakov [email protected]