Page MenuHomePhabricator

[AArch64][SVEIntrinsicOpts] Replace last{a,b} intrinsic calls with extracts...
ClosedPublic

Authored by joechrisellis on Wed, Apr 14, 6:38 AM.

Details

Summary

when the predicate used by last{a,b} specifies a known vector length.

For example:

aarch64_sve_lasta(VL1, D) -> extractelement(D, #1)
aarch64_sve_lastb(VL1, D) -> extractelement(D, #0)

Co-authored-by: Paul Walker <paul.walker@arm.com>

Diff Detail

Event Timeline

joechrisellis created this revision.Wed, Apr 14, 6:38 AM
joechrisellis requested review of this revision.Wed, Apr 14, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 14, 6:38 AM

Like D100463, could this be done in instcombine/TTI::instCombineIntrinsic?

Address review comments.

  • @dmgreen: move transformation to instcombine.
paulwalker-arm added inline comments.Fri, Apr 16, 5:02 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
292–296

Can the actual combine live in its own function? (for example into instCombineSVELast ) Otherwise this function is going to become unreadable?

joechrisellis marked an inline comment as done.Fri, Apr 16, 9:34 AM
joechrisellis added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
292–296

Sure! The only reason that I didn't do this the first time is because it didn't look like there was much precedent for breaking down the TTI::instCombineIntrinsic into specific combine functions for other targets. Happy to change if we want to do things differently though.

joechrisellis marked an inline comment as done.

Address review comments.

Thanks for the change. This looks very sensible to me, so long as the other SVE folks agree.

paulwalker-arm accepted this revision.Mon, Apr 19, 2:28 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
285–286

Are these needed? I can only see one use of either within the IsAfter block but then the following code falls back to II.getArgOperand calls. It'll be nice to unify to one or the other.

llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-lasta-lastb.ll
3

I suspect this is fallout from code we have downstream where a single test was used for opt and llc. Given we're not going to do that here can this be removed and thus have us fall back to the usual CHECK identifier?

This revision is now accepted and ready to land.Mon, Apr 19, 2:28 AM

Looks fine to me with a couple of issues that can be resolved before committing.

Address review comments.