[SYCL] Implement host pipe unique name generation and mapping calls #8009

rho180 · 2023-01-13T19:32:18Z

Implementation of host pipes outlined in the design document in this PR:

#5850

Generation a unique pipe id for GVs marked with the new "sycl-host-pipe" attribute. Id generation utilizes the same method as used for name generation for device global.
Added a host pipe map to map the addresses of marked GVs with the unique id. This host pipe map is generated by a constructor and method calls added to the header and footer.
Modified the sycl-post-link tool to generate compile time properties metadata for these GVs. This metadata contains the unique id generated for the GV to be consumed by the device backend compiler.

PR for accompanying runtime changes: #7468

smanna12 · 2023-01-13T20:19:23Z

Could you please add FE tests?

rho180 · 2023-01-16T12:58:11Z

Could you please add FE tests?

Yep, I'll do that

steffenlarsen · 2023-01-16T18:36:07Z

clang/include/clang/Basic/AttrDocs.td

+
+  __pipeType __pipe;
+  }];
+}


@elizabethandrews - Now that we have [[__sycl_detail__::sycl_type(...)]], could this be accomplished using it? If so, we may consider doing the same for [[__sycl_detail__::device_global]]

I think we can use sycl_type here. I have a JIRA to replace device_global to use sycl_type as well. Just haven't gotten around to it.

@elizabethandrews @steffenlarsen Are there examples or documentation for sycl_type? I took a quick look in the repo and it's not clear to me how I should use it here.

@rho180 you can see examples of use here - #6674. You just need to add host_pipe to the definition

@elizabethandrews @steffenlarsen Updated. Though I know it's not best practice, I copied the static function isSyclType() from SemaSYCL.cpp to do the type check in CodeGenModule. Let me know if there's a common place where this can be put and I can make that change as well.

steffenlarsen · 2023-01-16T18:38:49Z

clang/lib/Driver/ToolChains/Clang.cpp

@@ -9644,6 +9644,9 @@ void SYCLPostLink::ConstructJob(Compilation &C, const JobAction &JA,
  // Process device-globals.
  addArgs(CmdArgs, TCArgs, {"-device-globals"});

+  // Process host pipes.
+  addArgs(CmdArgs, TCArgs, {"-host-pipes"});


We are planning on moving the CompileTimePropertiesPass to be an LLVM pass rather than a sub-feature of sycl-post-link, in which case we won't need this. See #7527. Note also this may affect the sycl-post-link related changes in this PR, but it should be resolvable.

@steffenlarsen Now that the CompileTimePropertiesPass change was merged, I rebased, moved my host pipe change out of sycl-post-link, and removed the host-pipe arg from the driver.

Fznamznon

I'm wondering, what is the difference between a host pipe and a device global? Can we just use a device global of a particular type to define a host pipe instead?

clang/lib/CodeGen/CodeGenModule.cpp

clang/lib/Sema/SemaSYCL.cpp

rho180 · 2023-01-24T18:53:11Z

I'm wondering, what is the difference between a host pipe and a device global? Can we just use a device global of a particular type to define a host pipe instead?

Device globals involve creating storage on the device that is accessible by all kernels. Host pipes can be connected to only one kernel, and do not involve creating storage but are linked to specialized hardware. The commonality is simply that they both require a mechanism to translate addresses in host space to some logical name that the backend compiler knows about.

elizabethandrews

Sorry for the delay in review. This PR fell off my radar. FE changes look ok to me. Just a minor comment about test

clang/test/CodeGenSYCL/host_pipe.cpp

premanandrao

(sorry for the late review. My notifications weren't quite working.)
FE changes look okay to me.

clang/test/CodeGenSYCL/host_pipe_int_footer_header.cpp

Fznamznon

FE part LGTM.

smanna12

FE changes look OK to me

llvm/include/llvm/SYCLLowerIR/HostPipes.h

rho180 · 2023-02-06T17:12:09Z

@KseniyaTikhomirova I didn't mean to remove you from the review list, I'd only hit the "re-request" button for @steffenlarsen. I'm not sure who else needs to review this to enable merging.

steffenlarsen · 2023-02-06T17:15:20Z

Missing reviews from @intel/dpcpp-clang-driver-reviewers & @intel/dpcpp-tools-reviewers .

llvm/include/llvm/SYCLLowerIR/HostPipes.h

llvm/tools/sycl-post-link/sycl-post-link.cpp

KseniyaTikhomirova · 2023-02-06T17:43:05Z

@KseniyaTikhomirova I didn't mean to remove you from the review list, I'd only hit the "re-request" button for @steffenlarsen. I'm not sure who else needs to review this to enable merging.

Hello, do not worry. Steffen approval is enough on behalf of SYCL RT reviewers side😊

…st-link flag

AlexeySachkov

sycl-post-link part LGTM

llvm/include/llvm/SYCLLowerIR/SYCLUtils.h

Co-authored-by: Alexey Sachkov <[email protected]>

rho180 requested review from a team as code owners January 13, 2023 19:32

rho180 requested a review from KseniyaTikhomirova January 13, 2023 19:32

rho180 temporarily deployed to aws January 13, 2023 20:09 — with GitHub Actions Inactive

steffenlarsen self-requested a review January 16, 2023 09:17

steffenlarsen reviewed Jan 16, 2023

View reviewed changes

rho180 temporarily deployed to aws January 16, 2023 19:20 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 16, 2023 19:48 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 16, 2023 20:19 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 18, 2023 18:38 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 18, 2023 19:28 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 18, 2023 20:04 — with GitHub Actions Inactive

Fznamznon reviewed Jan 19, 2023

View reviewed changes

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

rho180 temporarily deployed to aws January 24, 2023 20:21 — with GitHub Actions Inactive

rho180 temporarily deployed to aws January 24, 2023 22:24 — with GitHub Actions Inactive

elizabethandrews reviewed Feb 1, 2023

View reviewed changes

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

rho180 temporarily deployed to aws February 1, 2023 18:46 — with GitHub Actions Inactive

premanandrao approved these changes Feb 1, 2023

View reviewed changes

elizabethandrews reviewed Feb 1, 2023

View reviewed changes

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

rho180 temporarily deployed to aws February 1, 2023 19:55 — with GitHub Actions Inactive

rho180 temporarily deployed to aws February 1, 2023 20:27 — with GitHub Actions Inactive

Fznamznon approved these changes Feb 2, 2023

View reviewed changes

elizabethandrews approved these changes Feb 2, 2023

View reviewed changes

smanna12 approved these changes Feb 2, 2023

View reviewed changes

Implement host pipe unique name generation and mapping calls

823985f

rho180 added 4 commits February 2, 2023 11:16

Simplify host_pipe attribute checking

e8845cc

Remove unnecessary lit test args for host_pipe test

d0fe353

Remove another unnecessary arg for host_pipe test

7b828d7

Move host pipe compile time properties processing to opt

e34ac9e

rho180 force-pushed the hostpipe-impl branch from 234b812 to e34ac9e Compare February 2, 2023 21:13

rho180 requested a review from a team as a code owner February 2, 2023 21:13

Remove -host-pipes arg from clang driver tests

9e28e16

rho180 temporarily deployed to aws February 2, 2023 21:46 — with GitHub Actions Inactive

v-klochkov reviewed Feb 2, 2023

View reviewed changes

llvm/include/llvm/SYCLLowerIR/HostPipes.h Outdated Show resolved Hide resolved

rho180 temporarily deployed to aws February 2, 2023 22:30 — with GitHub Actions Inactive

Cleanup unnecessary include and class declarations in HostPipes header

aad8b8c

v-klochkov approved these changes Feb 2, 2023

View reviewed changes

rho180 temporarily deployed to aws February 2, 2023 23:58 — with GitHub Actions Inactive

rho180 temporarily deployed to aws February 3, 2023 00:30 — with GitHub Actions Inactive

rho180 requested review from steffenlarsen and removed request for KseniyaTikhomirova February 6, 2023 13:48

steffenlarsen approved these changes Feb 6, 2023

View reviewed changes

AlexeySachkov reviewed Feb 6, 2023

View reviewed changes

llvm/include/llvm/SYCLLowerIR/HostPipes.h Outdated Show resolved Hide resolved

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

Move HostPipes check function to SYCLUtils; remove extraneous sycl-po…

dacaed3

…st-link flag

rho180 requested a review from AlexeySachkov February 6, 2023 18:14

rho180 temporarily deployed to aws February 6, 2023 19:06 — with GitHub Actions Inactive

AlexeySachkov approved these changes Feb 6, 2023

View reviewed changes

llvm/include/llvm/SYCLLowerIR/SYCLUtils.h Outdated Show resolved Hide resolved

Update llvm/include/llvm/SYCLLowerIR/SYCLUtils.h

82a1efa

Co-authored-by: Alexey Sachkov <[email protected]>

rho180 temporarily deployed to aws February 6, 2023 21:06 — with GitHub Actions Inactive

rho180 temporarily deployed to aws February 6, 2023 21:38 — with GitHub Actions Inactive

bader merged commit 752e4d3 into intel:sycl Feb 7, 2023

[SYCL] Implement host pipe unique name generation and mapping calls #8009

[SYCL] Implement host pipe unique name generation and mapping calls #8009

Uh oh!

Conversation

rho180 commented Jan 13, 2023

Uh oh!

smanna12 commented Jan 13, 2023

Uh oh!

rho180 commented Jan 16, 2023

Uh oh!

steffenlarsen Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

rho180 Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

rho180 Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

rho180 Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rho180 commented Jan 24, 2023

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rho180 commented Feb 6, 2023

Uh oh!

steffenlarsen commented Feb 6, 2023

Uh oh!

Uh oh!

Uh oh!

KseniyaTikhomirova commented Feb 6, 2023

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!