Skip to content

[SYCL] Initial printf support for non-constant AS format strings #5069

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 32 commits into from
Dec 22, 2021
Merged

[SYCL] Initial printf support for non-constant AS format strings #5069

merged 32 commits into from
Dec 22, 2021

Conversation

AGindinson
Copy link
Contributor

@AGindinson AGindinson commented Dec 2, 2021 •
edited
Loading

Allow generic address space strings into experimental::printf and, consequently,
into __spirv_ocl_printf declarations. To mitigate the lack of device BE support for
generic address space literals, we implement a pass to move the literal argument
into constant address space whenever possible.

This is a temporary solution; long-term, we should replace this with proper support
of generic address-spaced format strings at the level of SPIR-V translation and device
backends.

Tested in intel/llvm-test-suite#569.

Signed-off-by: Artem Gindinson [email protected]

AlexeySachkov
AlexeySachkov reviewed Dec 2, 2021
View reviewed changes
AlexeySachkov
AlexeySachkov reviewed Dec 7, 2021
View reviewed changes
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Overall the pass looks almost good to me, waiting for tests

Artem Gindinson added 7 commits December 8, 2021 13:42
Ensure correct llvm::Type* for literal & more refactoring
f134ee8 
Signed-off-by: Artem Gindinson <[email protected]>
Improve the function search
c16fcbb 
Signed-off-by: Artem Gindinson <[email protected]>
Ugly implementation for the O1 case
41eaf3b 
Signed-off-by: Artem Gindinson <[email protected]>
Extend value stripping to loads/stores for O0 cases
a26f7c8 
Signed-off-by: Artem Gindinson <[email protected]>
Replace call operands instead of re-creating calls
9150b8d 
+ bump up the size of `FunctionsToDrop` for the non-variadic case

Signed-off-by: Artem Gindinson <[email protected]>
Huge refactoring
f467434 
Signed-off-by: Artem Gindinson <[email protected]>
More refactoring & descriptive comments added
00066b6 
Signed-off-by: Artem Gindinson <[email protected]>
AGindinson
AGindinson commented Dec 9, 2021
View reviewed changes
AGindinson pushed a commit to AlexeySachkov/llvm-test-suite that referenced this pull request Dec 13, 2021
Test generic address space alongside constant AS
bee58c6 
Aims to test intel/llvm#5069.

Signed-off-by: Artem Gindinson <[email protected]>
@AGindinson
Copy link
Contributor Author

/verify with intel/llvm-test-suite#569

AGindinson
AGindinson commented Dec 13, 2021
View reviewed changes
Improve formatting
5c5b875 
Signed-off-by: Artem Gindinson <[email protected]>
@AGindinson
Copy link
Contributor Author

/verify with intel/llvm-test-suite#569

Artem Gindinson added 5 commits December 15, 2021 00:36
Avoid using the error-prone Module::getOrInsertGlobal w/ callback
e5ae867 
The API usage ended up breaking Module destruction in some cases by
leaving unregistered `Constant` users dangling. Also, the type was
being set incorrectly and had to be mutated explicitly.

Signed-off-by: Artem Gindinson <[email protected]>
Wrap the newly created globals into constant pointer casts if needed
39efbb4 
Signed-off-by: Artem Gindinson <[email protected]>
Replace the wrapper calls directly with builtin calls
4d1c6be 
This simplifies the pass implementation, the resulting IR, and also
achieves in-memory IR validity for no-inline cases.

Signed-off-by: Artem Gindinson <[email protected]>
Add LIT tests
87d4137 
Signed-off-by: Artem Gindinson <[email protected]>
Simplify call replacing (since we're always dealing with constants)
6c8122b 
Signed-off-by: Artem Gindinson <[email protected]>
AlexeySachkov
AlexeySachkov reviewed Dec 16, 2021
View reviewed changes
mlychkov
mlychkov reviewed Dec 17, 2021
View reviewed changes
@AGindinson
Copy link
Contributor Author

@AlexeySachkov @mlychkov @v-klochkov I'm thinking about moving the pass from SYCLLowerIR into Transforms/IPO to minimize code-ownership conflicts between DPC++ Tools and DPC++ ESIMD. I don't know if this would reflect the nature of the SYCL printf address space mutation as well, but since the pass is intended as temporary, short-term maintainability seems like a bigger focus. What are your thoughts on this?

@AlexeySachkov
Copy link
Contributor

@AlexeySachkov @mlychkov @v-klochkov I'm thinking about moving the pass from SYCLLowerIR into Transforms/IPO to minimize code-ownership conflicts between DPC++ Tools and DPC++ ESIMD. I don't know if this would reflect the nature of the SYCL printf address space mutation as well, but since the pass is intended as temporary, short-term maintainability seems like a bigger focus. What are your thoughts on this?

I would actually prefer if this pass resides in SYCLLowerIR, because it is easier for us to distinguish it from upstream passes. I'm less concerned about mixing regular DPC++ and DPC++ ESIMD passes than about mixing our customizations with upstream code

@AGindinson
Copy link
Contributor Author

/verify with intel/llvm-test-suite#569

@AGindinson AGindinson requested a review from mlychkov December 20, 2021 10:48
@AGindinson
Copy link
Contributor Author

@intel/llvm-reviewers-runtime, @v-klochkov, could you please approve changes to printf signatures in sycl/include/?
@kbobrovs, @kychendev, @sndmitriev, @v-klochkov, could someone please approve the addition of the pass files under llvm/**/SYCLLowerIR directories?
@elizabethandrews, @premanandrao, @smanna12, could someone please approve changes to Clang's pass scheduling in clang/lib/CodeGen/BackendUtil.cpp?
@bader, could you please approve the pass registration under llvm/?

(should #5016 come in earlier, I'll rebase the patch and rely on team-based code-owner approval)

bader
bader approved these changes Dec 20, 2021
View reviewed changes
elizabethandrews
elizabethandrews approved these changes Dec 20, 2021
View reviewed changes
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

FE change (add a pass) looks good to me.

@AGindinson
Copy link
Contributor Author

For the joint Precommit of product + test patches, CUDA/Win failures in DeviceLib tests are known to be unrelated.

@AGindinson AGindinson requested a review from a team December 22, 2021 06:34
@AlexeySachkov AlexeySachkov merged commit 2d62e51 into intel:sycl Dec 22, 2021
AGindinson pushed a commit to AGindinson/llvm that referenced this pull request Dec 22, 2021
[SYCL] Update CODEOWNERS after printf support was merged
ff82dfd 
Follow-up on intel#5069.

Signed-off-by: Artem Gindinson <[email protected]>
@AGindinson AGindinson mentioned this pull request Dec 22, 2021
Merged
AGindinson pushed a commit to AGindinson/llvm that referenced this pull request Dec 22, 2021
[SYCL] Fix --shared-libs builds for generic AS printf support
67dd4a1 
Fixes post-commit failures for intel#5069.

Signed-off-by: Artem Gindinson <[email protected]>
@AGindinson AGindinson mentioned this pull request Dec 22, 2021
Merged
bader pushed a commit that referenced this pull request Dec 22, 2021
[CI] Update CODEOWNERS for SYCL printf support passes (#5199)
8ef9cee 
Follow-up on #5069.

Signed-off-by: Artem Gindinson <[email protected]>
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 23, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into remove_this_func_fe
fa65690 
* upstream/sycl: (1382 commits)
  [SYCL][XPTI] Report memory allocation info from SYCL runtime (intel#5172)
  [CI] Switch labels for OCL x64 job (intel#5185)
  [SYCL] Add basic support for the generic_space address space (intel#5148)
  [CI] Update CODEOWNERS for SYCL printf support passes (intel#5199)
  [SYCL][Matrix] Enable wi_slice for joint_matrix (intel#4979)
  [SYCL][Group algorithms] Move group sort extension to experimental (intel#5169)
  [SYCL] Fix kernel bundles don't really carry kernel IDs (intel#5121)
  [SYCL] Initial printf support for non-constant AS format strings (intel#5069)
  [SYCL][NFC] Fix static code analysis concerns (intel#5189)
  [SYCL][Doc] Fix typos to fix doc build (intel#5190)
  [Driver][SYCL] Turn on -fsycl-dead-args-optimization by default (intel#3004)
  [SYCL][L0][Plugin] Add support for batching copy commands (intel#5155)
  [CI] Add cache checkout script to docker containers (intel#5184)
  [SYCL][Doc] Add HIP backend to the filter selector (intel#5176)
  [Doc] Add documentation for Docker images (intel#4778)
  [LIBCLC] Add functionality for in-kernel asserts for CUDA backend (intel#5174)
  Force opt to use new pass manager in exponential-deferred-inlining test after a8c2ba1
  [SYCL] Add vec and marray support to known_identity type trait (intel#5163)
  Correctly resolve merge conflicts
  Update SPV_INTEL_hw_thread_queries to rev 2
  ...
bader pushed a commit that referenced this pull request Dec 24, 2021
[SYCL] Fix --shared-libs builds for generic AS printf support (#5200)
04c18f2 
Fixes post-commit failures for #5069.

Signed-off-by: Artem Gindinson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@alexbatashev alexbatashev alexbatashev approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@kbobrovs kbobrovs kbobrovs approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@smanna12 smanna12 smanna12 approved these changes

@kychendev kychendev Awaiting requested review from kychendev

@premanandrao premanandrao Awaiting requested review from premanandrao

@sndmitriev sndmitriev Awaiting requested review from sndmitriev

@v-klochkov v-klochkov Awaiting requested review from v-klochkov

@mlychkov mlychkov Awaiting requested review from mlychkov

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

Successfully merging this pull request may close these issues.
8 participants
@AGindinson @AlexeySachkov @bader @alexbatashev @kbobrovs @elizabethandrews @mlychkov @smanna12