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

Add pass to lower Bitcast to nonstandard type instructions #1117

Merged
merged 23 commits into from
Aug 17, 2021
Merged

Add pass to lower Bitcast to nonstandard type instructions #1117

merged 23 commits into from
Aug 17, 2021

Conversation

KornevNikita
Copy link
Contributor

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 4
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.

@KornevNikita
Copy link
Contributor Author

@AlexeySachkov @AlexeySotkin @Fznamznon take a look please

lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
@Fznamznon
Copy link
Contributor

I didn't dig deeply into all loops, because for me recursive loop over users of faulty bitcast seems more convenient.
We also should remember that the pass like this, i.e. changing types can be very hard to maintain, since there is always more unhandled patterns coming.

@AlexeySotkin
Copy link
Contributor

We also should remember that the pass like this, i.e. changing types can be very hard to maintain, since there is always more unhandled patterns coming.

I can't think of a better solution to handle such types. @Fznamznon, if you have any ideas, please share.

lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
// doesn't like several nested instructions in one.
NewValue = new AddrSpaceCastInst(NewValue, NewVecPtrTy);
Builder.Insert(NewValue);
OldInstIter = ASCastInst;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are several addressspace casts which use the same instruction?

Copy link
Contributor Author

@KornevNikita KornevNikita Aug 3, 2021

Choose a reason for hiding this comment

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

Added TODO comment in 587cc7f

Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are several addressspace casts which use the same instruction?

You mean we don't need break at the end of the if? If yes, I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case whole function should be refactored to process several patterns "BitCast -> AScast -> Load -> ExtractElement" in an IR function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can't just remove the break, because the code assumes that there is only one instruction which needs to be fixed

lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
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.

I think that we can merge this as some kind of baseline, i.e. in general algorithm works, but in order for it to be useful in LLVM IR produced from real applications, we must ensure that we resolve TODO about multiple uses of non-standard vector types (from SPIRVLowerBitCastToNonStandardType.cpp:77)

lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
lib/SPIRV/SPIRVLowerBitCastToNonStandardType.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mlychkov mlychkov left a comment

Choose a reason for hiding this comment

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

Thank you very much for your patience and willingness to accept constructive remarks.
Now all looks good to me.

@KornevNikita
Copy link
Contributor Author

Clang-tidy warning about recursion can be ignored, because the variable "RecursionDepth" is added to control the depth, so it cannot loop.

@AlexeySotkin AlexeySotkin merged commit ea7468e into KhronosGroup:master Aug 17, 2021
AlexeySotkin pushed a commit to AlexeySotkin/SPIRV-LLVM-Translator that referenced this pull request Aug 20, 2021
…oup#1117)

* Add pass to lower Bitcast to nonstandard type instructions

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 1
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to  <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.
AlexeySotkin pushed a commit to AlexeySotkin/SPIRV-LLVM-Translator that referenced this pull request Aug 20, 2021
…oup#1117)

* Add pass to lower Bitcast to nonstandard type instructions

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 1
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to  <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.
AlexeySotkin pushed a commit to AlexeySotkin/SPIRV-LLVM-Translator that referenced this pull request Aug 20, 2021
…oup#1117)

* Add pass to lower Bitcast to nonstandard type instructions

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 1
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to  <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.
AlexeySotkin pushed a commit to AlexeySotkin/SPIRV-LLVM-Translator that referenced this pull request Aug 20, 2021
…oup#1117)

* Add pass to lower Bitcast to nonstandard type instructions

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 1
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to  <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.
AlexeySotkin pushed a commit that referenced this pull request Aug 23, 2021
* Add pass to lower Bitcast to nonstandard type instructions

It is a pass to lower bitcast instructions to non-standard SPIR-V types.
It covers only known issues. At this moment there is only one pattern
that should be covered - use of vector with unsupported number of
elements. This pattern should be handled this way:

%0 = bitcast <3 x i64> addrspace(1)* @id to <6 x i32> addrspace(1)*
%1 = addrspacecast <6 x i32> addrspace(1)* %0 to <6 x i32> addrspace(4)*
%2 = load <6 x i32>, <6 x i32> addrspace(4)* %1, align 32
%conv = extractelement <6 x i32> %2, i32 1
%conv1 = sitofp i32 %conv to float

Must be replaced by:
%0 = addrspacecast <3 x i64> addrspace(1)* @id to  <3 x i64> addrspace(4)*
%1 = load <3 x i64>, <3 x i64> addrspace(4)* %0, align 32
%2 = extractelement <3 x i64> %1, i32 0
%conv = trunc i64 %2 to i32
%conv1 = sitofp i32 %conv to float

It is assumed that the pass will be further developed as new patterns arise.
@KornevNikita KornevNikita deleted the pass branch October 8, 2021 07:53
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.

5 participants