Skip to content

[SYCL] Propagate kernel parameter optimization info to SYCL runtime. #2303

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 7 commits into from
Aug 14, 2020
Merged

[SYCL] Propagate kernel parameter optimization info to SYCL runtime. #2303

merged 7 commits into from
Aug 14, 2020

Conversation

kbobrovs
Copy link
Contributor

PLEASE DO NOT SQUASH

  • Implement Base64 encoder/decoder to represent arbitrary data in property files
  • Introduce BYTE_ARRAY property type in PropertySetIO
  • Implement support for BYTE_ARRAY properties in clang offload wrapper
  • Use the above tools to propagate kernel parameter optimization info from IR to SYCL RT:
    • The post-link tool
      • uses a new analysis pass (SPIRKernelParamOptInfo) to extract kernel parameter optimization info from IR produced by the DeadArgumentElimination pass
      • uses the PropertySetIO mechanism to encode this information into the output property file
    • Then the clang offload wrapper picks up the property file and injects it into the device binary descriptor accessed by the SYCL runtime when the app runs.

bader
bader reviewed Aug 12, 2020
View reviewed changes
@kbobrovs kbobrovs requested a review from bader August 12, 2020 13:09
@kbobrovs
Copy link
Contributor Author

@bader, ping

@kbobrovs
Implement generic Base64 encoder/decoder.
720173b 
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
Simplify error handling in PropertySetIO.cpp.
2946ec5 
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
Remove unused declaration from PropertySetIO.h
224056a 
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
Implement byte array property value.
29dea8d 
This property value is used to represent arbitrary binary data - array of bytes.
When stored in a property file, the data is encoded using Base64 algorithm to
ensure the file contains only printable ASCII characters.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
[SYCL] Support BYTE_ARRAY property in the offload wrapper.
25efdd3 
Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
[SYCL] Propagate SPIRV kernel parameter opt info to runtime.
36d4d36 
The post-link tool
- uses a new analysis pass (SPIRKernelParamOptInfo) to extract kernel
  parameter optimization info from IR produced by the DeadArgumentElimination
  pass
- uses the PropertySetIO mechanism to encode this information into the ourput
  property file
Then the clang offload wrapper picks up the property file and injects it into
the device binary descriptor accessed by the SYCL runtime when the app runs.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>
@kbobrovs
Copy link
Contributor Author

Addressed comments. Had to force-push to clean-up commit sequence. Now it can be committed as is w/o further squashing.

Fznamznon
Fznamznon reviewed Aug 14, 2020
View reviewed changes
llvm/test/tools/sycl-post-link/omit_kernel_args.ll
!0 = !{i1 true, i1 false}

; // HEX: 0x7E 0x06 (2 bytes)
; // BIN: 110 01111110 - 11 arguments total, 3 remain
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean that original kernel had 11 arguments, but only 3 of them are used?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just a bit strange, because I would expect "true" or "1" value for non-dead argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. But this is how the DeadArgElimination defines it

bader
bader previously approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

llvm/* changes look good to me.

PLEASE DO NOT SQUASH

Why not to squash?

@kbobrovs
Copy link
Contributor Author

Why not to squash?

For better granularity. Each commit implements a specific feature, and earlier ones don't depend on later ones.

@kbobrovs kbobrovs requested review from Fznamznon and bader August 14, 2020 15:02
Fznamznon
Fznamznon approved these changes Aug 14, 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.

sycl-post-link changes looks ok

Fznamznon
Fznamznon reviewed Aug 14, 2020
View reviewed changes
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -610,6 +610,15 @@ class BinaryWrapper {
return std::make_pair(ImageB, ImageE);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need code owners for clang-offload-wrapper/bundler tools.

@bader bader merged commit b0d98dc into intel:sycl Aug 14, 2020
@kbobrovs kbobrovs deleted the opt-kernel-params branch August 14, 2020 19:17
@bader bader mentioned this pull request Aug 22, 2020
Closed
jsji pushed a commit that referenced this pull request Jan 25, 2024
@LU-JOHN @sys-ce-bb
Revert change to Fortran array test (#2307)
ba6699d 
Revert change to Fortran array test. Also rewrite it to DAG checks to avoid output order problems in future.
Previous change was in:

> Author: Viktoria Maximova <[email protected]>
> Date:   Tue Jan 16 17:16:33 2024 +0100
>
>     Fix DebugInfo/NonSemantic/Shader200/FortranArray.ll (#2303)
>
>     The change is needed after llvm/llvm-project@fc6faa11
>

The LLVM change that motivated the change to FortranArray.ll has been reverted:

> Author: Davide Italiano <[email protected]>
> Date:   Tue Jan 16 16:56:24 2024 -0800
>
>     Revert "[CloneFunction][DebugInfo] Avoid cloning DILocalVariables of inlined functions (#75385)"
>
>     This reverts commit fc6faa1.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@7bf3fb4
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@callumfare
Merge pull request #2303 from nrspruit/zeInitDrivers
3a9eb11 
[L0] Implement Support for zeInitDrivers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fznamznon Fznamznon Fznamznon approved these changes

@bader bader bader approved these changes

@AGindinson AGindinson Awaiting requested review from AGindinson

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

@mdtoguchi mdtoguchi Awaiting requested review from mdtoguchi

@premanandrao premanandrao Awaiting requested review from premanandrao

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

Successfully merging this pull request may close these issues.
3 participants
@kbobrovs @bader @Fznamznon