Page MenuHomePhabricator

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

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



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
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?

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
@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:

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

nit: MinNumElts


This cast is redundant, because VecVTy is already a VectorType, see line 4475.


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.


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.


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.


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.