8000 [SYCL] Handle KernelName templated using type with enum template argument by elizabethandrews · Pull Request #1780 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL] Handle KernelName templated using type with enum template argument #1780

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 1, 2020
Merged

[SYCL] Handle KernelName templated using type with enum template argument #1780

merged 1 commit into from
Jun 1, 2020

Conversation

elizabethandrews
Copy link
Contributor
@elizabethandrews elizabethandrews commented May 29, 2020 •
edited
Loading

Add support to handle enums when KernelNameType is templated using a type which is in turn templated using enum.

Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com

@elizabethandrews
Copy link
Contributor Author
elizabethandrews commented May 29, 2020 •
edited
Loading

Please note: Enum used to template a 'template template type argument' has not been handled yet. I will open a second PR (or add a second commit here) for that. I am still working on it but thought I could get feedback on this patch for now.

@erichkeane
Copy link
Contributor

Haven't looked at the code yet, but based on the commit message, why is this doing 2 different tasks?

@elizabethandrews
Copy link
Contributor Author

Haven't looked at the code yet, but based on the commit message, why is this doing 2 different tasks?

I noticed the second issue while working on the first and just fixed it along with it. Should I open 2 different PRs or upload it as 2 separate commits?

erichkeane
erichkeane previously approved these changes May 29, 2020
View reviewed changes
@erichkeane
Copy link
Contributor

Haven't looked at the code yet, but based on the commit message, why is this doing 2 different tasks?

I noticed the second issue while working on the first and just fixed it along with it. Should I open 2 different PRs or upload it as 2 separate commits?

I'm on the fence, I'd have originally suggested doing it as 2 separate PRs (since they become quick/easy to review). That said, now that its up, I'd probably be satisfied if you were to reword the message to simply state why that was required to accomplish the task.

@elizabethandrews
Copy link
Contributor Author
elizabethandrews commented May 29, 2020 •
edited
Loading

Haven't looked at the code yet, but based on the commit message, why is this doing 2 different tasks?

I noticed the second issue while working on the first and just fixed it along with it. Should I open 2 different PRs or upload it as 2 separate commits?

I'm on the fence, I'd have originally suggested doing it as 2 separate PRs (since they become quick/easy to review). That said, now that its up, I'd probably be satisfied if you were to reword the message to simply state why that was required to accomplish the task.

I don't think this was required to accomplish the task. I'll check and move it out into a separate PR

@elizabethandrews
Add support to handle enums when KernelNameType is templated
964bb7b 
using a type which is in turn templated using enum.

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews elizabethandrews force-pushed the TypeArgTemplatedByEnum branch from 5cd3667 to 964bb7b Compare May 29, 2020 17:10
@elizabethandrews
Copy link
Contributor Author

Removed changes related to global specifier. Will upload this as second PR shortly.
8000

erichkeane
erichkeane reviewed May 29, 2020
View reviewed changes
clang/test/CodeGenSYCL/kernelname-enum.cpp
@@ -155,3 +182,5 @@ int main() {
// CHECK: template <> struct KernelInfo<::dummy_functor_6<(unscoped_enum)0>>
// CHECK: template <> struct KernelInfo<::dummy_functor_7<::no_namespace_int>>
// CHECK: template <> struct KernelInfo<::dummy_functor_7<::internal::namespace_short>>
// CHECK: template <> struct KernelInfo<::T1<::T2<(type_argument_template_enum::E)0>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

This inconsistency is fixed in the followup patch, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Fznamznon
Fznamznon approved these changes Jun 1, 2020
View reviewed changes
Copy link
Contributor
@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Test fails seem unrelated.

@bader
Copy link
Contributor
bader commented Jun 1, 2020

Test fails seem unrelated.

@vladimirlaz, @romanovvlad, do you know what might be the reason of "unrelated test fails"?
http://ci.llvm.intel.com:8010/#/builders/18/builds/2293/steps/16/logs/stdio

@elizabethandrews
Copy link
Contributor Author

Test fails seem unrelated.

@vladimirlaz, @romanovvlad, do you know what might be the reason of "unrelated test fails"?
http://ci.llvm.intel.com:8010/#/builders/18/builds/2293/steps/16/logs/stdio

Attaching logs for fails below. I am going to rebuild the test to check if it is flakey.

FAIL__SYCL__device_event_cpp.txt
FAIL__SYCL__mixed2template_cpp.txt

@bader bader merged commit f9226d2 into intel:sycl Jun 1, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jun 2, 2020
Merge remote-tracking branch 'upstream/sycl' into dump_record_layout
fc3b63d 
* upstream/sycl:
  [SYCL] Handle KernelName templated using type with enum template argument (intel#1780)
  [SYCL] Fix KernelNameInfo generated for empty template parameter pack (intel#1775)
  [SYCL] Do not export utility methods from SYCLMemObjT (intel#1768)
  [Driver][SYCL] Fix processing when using -fsycl-link (intel#1765)
  [SYCL][NFC] Remove outdated confusing comment (intel#1779)
  [SYCL][NFC] Wrap classes in .cpp into a namespace to disable external linkage. (intel#1776)
  [SYCL][CUDA] Fixes CUDA unit tests that uses SYCL directly (intel#1763)
  [SYCL][Doc] Fix default device selection rules doc (intel#1769)
  [SYCL][CUDA] Remove pi Event Callback implementation (intel#1735)
  [SYCL] Throw exception if range/offset of kernel execution exceeds INT_MAX (intel#1713)
  [SYCL-PTX] Add intermediate layer to libclc to ease type management (intel#1712)
bb-sycl pushed a commit that referenced this pull request Jan 18, 2023
@sarnex @KornevNikita
Translate atomicrmw fsub into FNegate and AtomicFAddEXT (#1780)
077c20c 
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@f95a6d7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erichkeane erichkeane erichkeane approved these changes

@Fznamznon Fznamznon Fznamznon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elizabethandrews @erichkeane @bader @Fznamznon
0