This patch allows that scalable vector can also use the fold that already
exists for fixed vector, only when the lane index is lower than the minimum
number of elements of the vector.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
- Add implementation and test to simplify constants
- Change check for splat in visitExtractElement
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
875–879 ↗ | (On Diff #347632) | It looks like this handles some of the same cases as the "CAZ of ScalableVectorType" case below. Can you combine them? |
llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp | ||
350 ↗ | (On Diff #347632) | This needs a comment explaining what case you are trying to handle, e.g. "(extractelement (shufflevector ...), ...) -> (...)" |
351 ↗ | (On Diff #347632) | You can dyn_cast SrcVec directly to ShuffleVectorInst, no need to go via Instruction. |
Could this be 2 independent patches?
- Enhancement to ConstantFold for scalable vectors
- Add a fold to llvm::SimplifyExtractElementInst() when the base vector is known to be a splat.
If that's right, we should move the tests to test/Transforms/InstSimplify/{ConstProp}, so we're checking the minimum required analysis. I don't think we need to touch InstCombine at all.
Thank you @foad and @spatel for the fast review.
I have splited the patch in 2 as @spatel suggested.
The constant fold is in https://reviews.llvm.org/D103180.
@spatel let me know if I understood correct what you've asked.
This is not what I had in mind. If we are not creating a new instruction, then we should try to get this into -instsimplify:
https://github.com/llvm/llvm-project/blob/6c92215e07f41cb566d54599e891180fe2ddbea5/llvm/lib/Analysis/InstructionSimplify.cpp#L4492
That's where we handle fixed-length vector splat+extract. Can we add to or adjust the underlying analysis there to handle scalable vectors?
Hi @spatel,
Sorry for getting wrong in the first place.
I hope I get it correct now with your latest explanation.
I changed the splat test to be in SimplifyExtractElementInst (InstructionSimplify.cpp).
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
4498–4499 | nit: MinNumElts | |
4499 | This cast is redundant, because VecVTy is already a VectorType, see line 4475. | |
4504–4507 | Why not just rely on llvm::getSplatValue here? unsigned MinNumElts = VecVTy->getElementCount().getKnownMinValue(); if (IdxC->getValue().ult(NumElts)) if (auto *Splat = getSplatValue(Vec)) return Splat; |
Thank you @sdesmalen for you review.
I have made the changes you've asked.
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
4504–4507 | Yes, it could check for Splat. |
Thanks, LGTM with my nit addressed.
llvm/lib/Analysis/InstructionSimplify.cpp | ||
---|---|---|
4501 | nit: Handle case where an element is extracted from a splat. |
Seems you just rebased, so it still looks fine to me.
llvm/test/Transforms/InstCombine/gep-vector-indices.ll | ||
---|---|---|
64 | Nice improvement. |
nit: MinNumElts