Page MenuHomePhabricator

[InstCombine] Add instcombine fold for extractelement + splat for scalable vectors
ClosedPublic

Authored by CarolineConcatto on May 13 2021, 7:58 AM.

Details

Summary

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.

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.May 13 2021, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 7:58 AM
Matt added a subscriber: Matt.May 21 2021, 8:11 AM
  • Add implementation and test to simplify constants
  • Change check for splat in visitExtractElement
foad added inline comments.Tue, May 25, 5:06 AM
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?

  1. Enhancement to ConstantFold for scalable vectors
  2. 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.

  • address reviewer's comment
  • reduce patch scope to only work with instcombine
CarolineConcatto marked 2 inline comments as done.Wed, May 26, 9:39 AM

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?

-move splat test from instcombine to instsimplify

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).

sdesmalen added inline comments.Tue, Jun 1, 7:41 AM
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;
CarolineConcatto marked 3 inline comments as done.
  • Address Sander's comments

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.
I like the other way because it explains/shows the instruction chain.
But your solution is cleaner, so I have changed.

sdesmalen accepted this revision.Wed, Jun 2, 8:42 AM

Thanks, LGTM with my nit addressed.

llvm/lib/Analysis/InstructionSimplify.cpp
4501

nit: Handle case where an element is extracted from a splat.

This revision is now accepted and ready to land.Wed, Jun 2, 8:42 AM
  • Replace comment as suggested
  • update test gep-vector-indices.ll

Seems you just rebased, so it still looks fine to me.

llvm/test/Transforms/InstCombine/gep-vector-indices.ll
64

Nice improvement.

This revision was landed with ongoing or failed builds.Tue, Jun 8, 2:45 AM
This revision was automatically updated to reflect the committed changes.