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

[SPV-IR to OCL] Fix mutated builtin call attribute copying #2208

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Nov 8, 2023

When SPIRVToOCL20Pass translates following SPV IR

  %call = tail call spir_func noundef ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4) noundef %Ptr, i32 noundef 0)
declare spir_func ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4), i32)

to OCL IR:

  %call = tail call spir_func noundef ptr @__to_private(ptr addrspace(4) noundef %Ptr)
declare spir_func ptr @__to_private(ptr addrspace(4))

, there is error Attribute after last parameter at copying attribute
to the new call because the old call has an additional AttributeSet for
the last argument. The last argument is eliminated in the new call.

Set tail call for new call if old call is tail call.

Revert cafd7e0. Partially revert c60327f.

When SPIRVToOCL20Pass translates following SPV IR
```
  %call = tail call spir_func noundef ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4) noundef %Ptr, i32 noundef 0)
declare spir_func ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4), i32)
```
to OCL IR:
```
  %call = tail call spir_func noundef ptr @__to_private(ptr addrspace(4) noundef %Ptr)
declare spir_func ptr @__to_private(ptr addrspace(4))
```
, there is error `Attribute after last parameter` at copying attribute
to the new call because the old call has an additional AttributeSet for
the last argument. The last argument is eliminated.

Set tail call for new call if old call is tail call.

Revert cafd7e0.

Test isn't added since I can only reproduce using SPIRVToOCL20Pass.
@MrSidims MrSidims requested a review from asudarsa November 8, 2023 22:21
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.

  %call = tail call spir_func noundef ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4) noundef %Ptr, i32 noundef 0)
declare spir_func ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4), i32)
to OCL IR:

  %call = tail call spir_func noundef ptr @__to_private(ptr addrspace(4) noundef %Ptr)
declare spir_func ptr @__to_private(ptr addrspace(4))

Can this be used as a test? You can add .spt file with usage of OpGenericCastToPtrExplicit instruction, translate it back like llvm-spirv -r --spirv-target-env="CL2.0"

lib/SPIRV/SPIRVBuiltinHelper.cpp Show resolved Hide resolved
lib/SPIRV/SPIRVBuiltinHelper.cpp Show resolved Hide resolved
@wenju-he
Copy link
Contributor Author

  %call = tail call spir_func noundef ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4) noundef %Ptr, i32 noundef 0)
declare spir_func ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4), i32)
to OCL IR:

  %call = tail call spir_func noundef ptr @__to_private(ptr addrspace(4) noundef %Ptr)
declare spir_func ptr @__to_private(ptr addrspace(4))

Can this be used as a test? You can add .spt file with usage of OpGenericCastToPtrExplicit instruction, translate it back like llvm-spirv -r --spirv-target-env="CL2.0"

Can noundef be represented in .spt file? If not, callinst argument won't have it in spirv and the failure won't be triggered w/o this pr.
I didn't find an alterative attribute listed in https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_function_parameter_attribute that can be applied to the second argument of the call.

@wenju-he
Copy link
Contributor Author

  %call = tail call spir_func noundef ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4) noundef %Ptr, i32 noundef 0)
declare spir_func ptr @_Z42__spirv_GenericCastToPtrExplicit_ToPrivatePvi(ptr addrspace(4), i32)
to OCL IR:

  %call = tail call spir_func noundef ptr @__to_private(ptr addrspace(4) noundef %Ptr)
declare spir_func ptr @__to_private(ptr addrspace(4))

Can this be used as a test? You can add .spt file with usage of OpGenericCastToPtrExplicit instruction, translate it back like llvm-spirv -r --spirv-target-env="CL2.0"

@MrSidims I've added above LIT and use opt to run.

@wenju-he wenju-he requested a review from MrSidims January 11, 2024 01:04
@MrSidims
Copy link
Contributor

Restarting CI with a new HEAD

@MrSidims MrSidims closed this Jan 29, 2024
@MrSidims MrSidims reopened this Jan 29, 2024
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.

Approved, giving a day for a community feedback (if any) before merge

@MrSidims MrSidims requested a review from svenvh January 29, 2024 18:04
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really a transcoding test (i.e., doesn't go LLVM IR -> SPIR-V -> LLVM IR), so I'd suggest moving it to a different place (outside of transcoding/ at least, top level would be fine with me for now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, moved to top level

@wenju-he wenju-he requested a review from svenvh January 31, 2024 04:19
@wenju-he
Copy link
Contributor Author

wenju-he commented Feb 5, 2024

@MrSidims could you please help to merge, thank you!

@MrSidims MrSidims merged commit a31a0a6 into KhronosGroup:main Feb 5, 2024
9 checks passed
MiloszSkobejko added a commit to MiloszSkobejko/SPIRV-LLVM-Translator that referenced this pull request Mar 13, 2024
MiloszSkobejko added a commit to MiloszSkobejko/SPIRV-LLVM-Translator that referenced this pull request Mar 19, 2024
Changes were cherry-picked from the following commit:
KhronosGroup@c6fe12b

Also added fixes from:
KhronosGroup#2208
KhronosGroup#2192

This changes add SPIR-V translator support for the SPIR-V extension documented here: KhronosGroup/SPIRV-Registry#193.
This extension adds one decoration to represent maximum error for FP operations and adds the related Capability.
SPIRV Headers support for representing this in SPIR-V: KhronosGroup/SPIRV-Headers#363

intel/llvm#8134 added a new call-site attribute associated with FP builtin intrinsics. This attribute is named 'fpbuiltin-max-error'.
Following example shows how this extension is supported in the translator. The input LLVM IR uses new LLVM builtin calls to represent FP operations. An attribute named 'fpbuiltin-max-error' is used to represent the max-error allowed in the FP operation.
Example
Input LLVM:
%t6 = call float @llvm.fpbuiltin.sin.f32(float %f1) KhronosGroup#2
attributes KhronosGroup#2 = { "fpbuiltin-max-error"="2.5" }

This is translated into a SPIR-V instruction (for add/sub/mul/div/rem) and OpenCl extended instruction for other instructions. A decoration to represent the max-error is attached to the SPIR-V instruction.

SPIR-V code:
4 Decorate 97 FPMaxErrorDecorationINTEL 1075838976
6 ExtInst 2 97 1 sin 88

No new support is added to support translating this SPIR_V back to LLVM. Existing support is used. The decoration is translated back into named metadata associated with the LLVM instruction. This can be readily consumed by backends.

Based on input from @andykaylor, we emit attributes when the FP operation is translated back to a call to a builtin function and emit metadata otherwise.

Translated LLVM code for basic math functions (add/sub/mul/div/rem):
%t6 = fmul float %f1, %f2, !fpbuiltin-max-error !7
!7 = !{!"2.500000"}

Translated LLVM code for other math functions:
%t6 = call spir_func float @_Z3sinf(float %f1) KhronosGroup#3
attributes KhronosGroup#3 = { "fpbuiltin-max-error"="4.000000" }
MiloszSkobejko added a commit to MiloszSkobejko/SPIRV-LLVM-Translator that referenced this pull request Mar 19, 2024
Changes were cherry-picked from the following commit:
KhronosGroup@c6fe12b

Also cherry picked fixes from:
KhronosGroup#2208
KhronosGroup#2192

This changes add SPIR-V translator support for the SPIR-V extension documented here: KhronosGroup/SPIRV-Registry#193.
This extension adds one decoration to represent maximum error for FP operations and adds the related Capability.
SPIRV Headers support for representing this in SPIR-V: KhronosGroup/SPIRV-Headers#363

intel/llvm#8134 added a new call-site attribute associated with FP builtin intrinsics. This attribute is named 'fpbuiltin-max-error'.
Following example shows how this extension is supported in the translator. The input LLVM IR uses new LLVM builtin calls to represent FP operations. An attribute named 'fpbuiltin-max-error' is used to represent the max-error allowed in the FP operation.
Example
Input LLVM:
%t6 = call float @llvm.fpbuiltin.sin.f32(float %f1) KhronosGroup#2
attributes KhronosGroup#2 = { "fpbuiltin-max-error"="2.5" }

This is translated into a SPIR-V instruction (for add/sub/mul/div/rem) and OpenCl extended instruction for other instructions. A decoration to represent the max-error is attached to the SPIR-V instruction.

SPIR-V code:
4 Decorate 97 FPMaxErrorDecorationINTEL 1075838976
6 ExtInst 2 97 1 sin 88

No new support is added to support translating this SPIR_V back to LLVM. Existing support is used. The decoration is translated back into named metadata associated with the LLVM instruction. This can be readily consumed by backends.

Based on input from @andykaylor, we emit attributes when the FP operation is translated back to a call to a builtin function and emit metadata otherwise.

Translated LLVM code for basic math functions (add/sub/mul/div/rem):
%t6 = fmul float %f1, %f2, !fpbuiltin-max-error !7
!7 = !{!"2.500000"}

Translated LLVM code for other math functions:
%t6 = call spir_func float @_Z3sinf(float %f1) KhronosGroup#3
attributes KhronosGroup#3 = { "fpbuiltin-max-error"="4.000000" }
MrSidims pushed a commit that referenced this pull request Mar 21, 2024
Changes were cherry-picked from the following commit:
c6fe12b

Also cherry picked fixes from:
#2208
#2192

This changes add SPIR-V translator support for the SPIR-V extension documented here: KhronosGroup/SPIRV-Registry#193.
This extension adds one decoration to represent maximum error for FP operations and adds the related Capability.
SPIRV Headers support for representing this in SPIR-V: KhronosGroup/SPIRV-Headers#363

intel/llvm#8134 added a new call-site attribute associated with FP builtin intrinsics. This attribute is named 'fpbuiltin-max-error'.
Following example shows how this extension is supported in the translator. The input LLVM IR uses new LLVM builtin calls to represent FP operations. An attribute named 'fpbuiltin-max-error' is used to represent the max-error allowed in the FP operation.
Example
Input LLVM:
%t6 = call float @llvm.fpbuiltin.sin.f32(float %f1) #2
attributes #2 = { "fpbuiltin-max-error"="2.5" }

This is translated into a SPIR-V instruction (for add/sub/mul/div/rem) and OpenCl extended instruction for other instructions. A decoration to represent the max-error is attached to the SPIR-V instruction.

SPIR-V code:
4 Decorate 97 FPMaxErrorDecorationINTEL 1075838976
6 ExtInst 2 97 1 sin 88

No new support is added to support translating this SPIR_V back to LLVM. Existing support is used. The decoration is translated back into named metadata associated with the LLVM instruction. This can be readily consumed by backends.

Based on input from @andykaylor, we emit attributes when the FP operation is translated back to a call to a builtin function and emit metadata otherwise.

Translated LLVM code for basic math functions (add/sub/mul/div/rem):
%t6 = fmul float %f1, %f2, !fpbuiltin-max-error !7
!7 = !{!"2.500000"}

Translated LLVM code for other math functions:
%t6 = call spir_func float @_Z3sinf(float %f1) #3
attributes #3 = { "fpbuiltin-max-error"="4.000000" }
@wenju-he wenju-he deleted the BuiltinCallMutator-attrs-copy branch May 16, 2024 07:46
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.

3 participants