-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[ConstantFolding] Add folding for [de]interleave2, insert and extract #141301
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
Changes from 6 commits
0145c57
6a37fdc
6c34f44
1145582
386bb29
77952a5
f64de79
ab1abeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1635,6 +1635,10 @@ bool llvm::canConstantFoldCallTo(const CallBase *Call, const Function *F) { | |||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_reduce_smax: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_reduce_umin: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_reduce_umax: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_extract: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_insert: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_interleave2: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_deinterleave2: | ||||||||||||||||||||||||||||||||||||||||
| // Target intrinsics | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::amdgcn_perm: | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::amdgcn_wave_reduce_umin: | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -3750,6 +3754,84 @@ static Constant *ConstantFoldFixedVectorCall( | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| return nullptr; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| case Intrinsic::vector_extract: { | ||||||||||||||||||||||||||||||||||||||||
| auto *Idx = dyn_cast<ConstantInt>(Operands[1]); | ||||||||||||||||||||||||||||||||||||||||
| Constant *Vec = Operands[0]; | ||||||||||||||||||||||||||||||||||||||||
| if (!Idx || !isa<FixedVectorType>(Vec->getType())) | ||||||||||||||||||||||||||||||||||||||||
| return nullptr; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| unsigned NumElements = FVTy->getNumElements(); | ||||||||||||||||||||||||||||||||||||||||
| unsigned VecNumElements = | ||||||||||||||||||||||||||||||||||||||||
| cast<FixedVectorType>(Vec->getType())->getNumElements(); | ||||||||||||||||||||||||||||||||||||||||
npanchen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
| unsigned StartingIndex = Idx->getZExtValue(); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Extracting entire vector is nop | ||||||||||||||||||||||||||||||||||||||||
| if (NumElements == VecNumElements && StartingIndex == 0) | ||||||||||||||||||||||||||||||||||||||||
| return Vec; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const unsigned NonPoisonNumElements = | ||||||||||||||||||||||||||||||||||||||||
| std::min(StartingIndex + NumElements, VecNumElements); | ||||||||||||||||||||||||||||||||||||||||
| for (unsigned I = StartingIndex; I < NonPoisonNumElements; ++I) { | ||||||||||||||||||||||||||||||||||||||||
| Constant *Elt = Vec->getAggregateElement(I); | ||||||||||||||||||||||||||||||||||||||||
| if (!Elt) | ||||||||||||||||||||||||||||||||||||||||
| return nullptr; | ||||||||||||||||||||||||||||||||||||||||
| Result[I - StartingIndex] = Elt; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| const unsigned NonPoisonNumElements = | |
| std::min(StartingIndex + NumElements, VecNumElements); | |
| for (unsigned I = StartingIndex; I < NonPoisonNumElements; ++I) { | |
| Constant *Elt = Vec->getAggregateElement(I); | |
| if (!Elt) | |
| return nullptr; | |
| Result[I - StartingIndex] = Elt; | |
| } | |
| for (unsigned I = 0; I < NumElements; ++I) { | |
| // Out of bounds elements are poison | |
| if (StartingIndex + I >= VecNumElements) { | |
| Result[I] = PoisonValue::get(FVTy->getElementType()); | |
| continue; | |
| } | |
| Constant *Elt = Vec->getAggregateElement(StartingIndex + I); | |
| if (!Elt) | |
| return nullptr; | |
| Result[I] = Elt; | |
| } |
npanchen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
npanchen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
lukel97 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
topperc marked this conversation as resolved.
Show resolved
Hide resolved
npanchen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SubVecNumElements is always >= 1 so if IdxN >= VecNumElements then IdxN + SubVecNumElements > VecNumElements. So I think you can remove the first check
| if (IdxN >= VecNumElements || IdxN + SubVecNumElements > VecNumElements || | |
| if (IdxN + SubVecNumElements > VecNumElements || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is needed to handle large indexes that could lead to unsigned wraparound. For example, there's added test fold_vector_insert_poison_large_idx to highlight this
topperc marked this conversation as resolved.
Show resolved
Hide resolved
npanchen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,100 @@ | ||||||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | ||||||
| ; RUN: opt < %s -passes=instsimplify,verify -disable-verify -S | FileCheck %s | ||||||
|
||||||
| ; RUN: opt < %s -passes=instsimplify,verify -disable-verify -S | FileCheck %s | |
| ; RUN: opt < %s -passes=instsimplify -S | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disable-verify needs to test poison value generation of vector.insert, vector.extract when writing/reading OOB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the verifier won't accept it does that mean the IR is actually malformed and not allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insert_value and extract_value both say something like
Elements idx through (idx + num_elements(subvec) - 1) must be valid vec indices. If this condition cannot be determined statically but is false at runtime, then the result vector is a [poison value](https://llvm.org/docs/LangRef.html#poisonvalues).
The only case it cannot be determined statically would be for extracting a fixed vector from a scalable vector or inserting a fixed vector into a scalable vector. For all other cases the indices must be in bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comes from #141301 (comment) comment.
The reason I think it's valid is if such instruction was constructed and being folded within a single pass, verification won't catch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still illegal IR and it should be considered a bug in any pass that created it.
@nikic what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's still invalid and we should not handle or test invalid LLVM IR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. Removed, so it came back to original changeset
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deinterleav->deinterleave
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deinterleav->deinterleave
Uh oh!
There was an error while loading. Please reload this page.