Skip to content
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

Translate OpIAddCarry and OpISubBorrow back to llvm intrinsics #2877

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

vmaksimo
Copy link
Contributor

Semantically we can translate OpIAddCarry and OpISubBorrow back into @llvm.uadd.with.overflow and @llvm.usub.with.overflow respectively. It require small transformation as there is a difference between return types in SPIR-V and LLVM IR - e.g., SPIR-V instructions return {i32, i32} struct, but LLVM intrinsics return {i32, i1}.

Semantically we can translate `OpIAddCarry` and `OpISubBorrow` back into `@llvm.uadd.with.overflow` and `@llvm.usub.with.overflow` respectively.
It require small transformation as there is a difference between return
types in SPIR-V and LLVM IR - e.g., SPIR-V instructions return {i32, i32} struct, but LLVM intrinsics return {i32, i1}.
@VyacheslavLevytskyy
Copy link
Contributor

CC: @maarquitos14

@maarquitos14
Copy link
Contributor

Just a nit: LLVM guidelines require proper punctuation in comments (https://llvm.org/docs/CodingStandards.html#commenting).

@vmaksimo
Copy link
Contributor Author

@maarquitos14 could you please point out where and what changes should be made? e.g. using suggestions in code review

lib/SPIRV/SPIRVReader.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVReader.cpp Outdated Show resolved Hide resolved
Co-authored-by: Marcos Maronas <marcos.maronas@intel.com>
@bwlodarcz
Copy link
Contributor

@vmaksimo @VyacheslavLevytskyy @maarquitos14 What is the rationale behind this change? During forward translation from llvm.uadd/usub.with.overflow to OpIAddCarry/OpISubBorrow, we encounter a nearly pathological situation where conversion between types like {i32, i1} and {i32, i32} occurs. This conversion is costly for the user but is necessary for support (we need to accept valid intrinsics from frontends). The reverse translation appears to make the same costly trade-off and doesn't have the same requirement as forward.

@vmaksimo Overall PR LGTM.

@VyacheslavLevytskyy
Copy link
Contributor

@llvm.uadd.with.overflow leads to insertion of __spirv_IAddCarry, and it's not something that run-time knows about. Any @llvm.uadd.with.overflow in the input now means a crash during run-time. This PR is to fix this by producing @llvm.uadd.with.overflow in the reverse translation.

@bwlodarcz
Copy link
Contributor

@VyacheslavLevytskyy Could you clarify which runtime you're referring to? SPIR-V supports multiple runtimes (e.g., SYCL, OpenCL, Vulkan, etc.). If a change leads to a crash in one of the runtimes, it would indicate a bug within that specific runtime, rather than a reason to reduce usability for everyone else.

@MrSidims MrSidims requested a review from svenvh November 27, 2024 12:52
@VyacheslavLevytskyy
Copy link
Contributor

The PR improves usability by producing a meaningful output of @llvm.uadd.with.overflow instead of a missing builtin __spirv_IAddCarry.

@bwlodarcz
Copy link
Contributor

@VyacheslavLevytskyy If the backends did not adopt the built-in solution and it is non-standard, that is acceptable.

Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

LGTM, no objections.

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

As I've mentioned in offline discussion ideally we should separate built-in like translation from intrinsic-like translation under an option as in this case it might affect performances on GPU. This can be done separately.

@MrSidims MrSidims merged commit 44bd69e into KhronosGroup:main Nov 28, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants