Page MenuHomePhabricator

[AArch64][SVE] Better utilisation of unpredicated forms of arithmetic intrinsics
ClosedPublic

Authored by bsmith on Apr 22 2021, 6:50 AM.

Details

Summary

When using predicated arithmetic intrinsics, if the predicate used is all
lanes active, use an unpredicated form of the instruction, additionally
this allows for better use of immediate forms.

This also includes a new complex isel pattern which allows matching an
all active predicate when the types are different but the predicate is a
superset of the type being used. For example, to allow a b8 ptrue for a
b32 predicate operand.

This only includes instructions where the unpredicated/predicated forms
are mismatched between variants, meaning that the removal of the
predicate is done during instruction selection in order to prevent
spurious re-introductions of ptrue instructions.

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

Diff Detail

Event Timeline

bsmith created this revision.Apr 22 2021, 6:50 AM
bsmith requested review of this revision.Apr 22 2021, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 6:50 AM
bsmith updated this revision to Diff 339627.Apr 22 2021, 7:41 AM
  • Remove warning check from test as it is no longer necessary
Matt added a subscriber: Matt.Apr 25 2021, 6:30 AM
sdesmalen added inline comments.Apr 26 2021, 9:26 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4994

I had expected there to be only a single REINTERPRET_CAST when it gets to the instruction selection phase, but I guess there are no combines that remove successive REINTERPRET_CASTs at the moment?

llvm/lib/Target/AArch64/SVEInstrFormats.td
346

should this pattern also use SVEAllActive?

354

You're probably just following precedent, but I find the naming here a bit confusing since I'd think _Pat suggests the pattern takes a predicate. In this case, my preference would be to pass the (SVEAllActive) as operand/patfrag to the pattern class.

bsmith added inline comments.Apr 27 2021, 3:57 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4994

I'm not actually sure if there will always only be a single cast or not, but it feels like it's probably safer to just assume there isn't as it doesn't really add any complexity.

llvm/lib/Target/AArch64/SVEInstrFormats.td
354

Perhaps changing this to be SVE_1_Op_Pred_Imm_Log_Pat would be better? (And changing the equivalent Arith version above). Passing in (SVEAllActive) in as a parameter just seems like it's adding parameters for the sake of it as nothing needs to pass this in differently.

bsmith updated this revision to Diff 340832.Apr 27 2021, 7:11 AM
bsmith marked 2 inline comments as done.
bsmith retitled this revision from [AArch64][SVE] Better utilisation of immediate forms for bitwise intrinsics to [AArch64][SVE] Better utilisation of immediate forms for bitwise/arith intrinsics.
bsmith edited the summary of this revision. (Show Details)
  • Rename multiclasses to use All_Active naming
  • Extend patch to cover some similar arith instructions
paulwalker-arm added inline comments.Apr 27 2021, 7:31 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
345–347

I'm surprised these changes are required. This suggests convertMergedOpToPredOp needs updating?

bsmith updated this revision to Diff 341130.Apr 28 2021, 3:38 AM
bsmith marked an inline comment as done.
  • Use all active predicate logic in convertMergedOpToPredOp for arith nodes
  • Remove intrinsic patterns for arith nodes since the above will convert them to ISD nodes, which already have patterns
bsmith updated this revision to Diff 341231.Apr 28 2021, 9:10 AM
  • Fixup some minor formatting issues
bsmith added inline comments.Apr 28 2021, 9:13 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13865

I very much dislike the blatant duplication of this function between here and ISelDAGToDAG, however I couldn't find anywhere sensible to share code between the two (and there is already a comment like this elsewhere in this file). Any pointers for a suitable place for this function would be much appreciated!

sdesmalen added inline comments.Apr 29 2021, 2:40 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13865

In AArch64DAGToDAGISel you can call getTargetLowering(), which returns a const TargetLowering *. This can be casted to const AArch64TargetLowering* which in turn can be used to access AArch64TargetLowering::isAllActivePredicate.

bsmith updated this revision to Diff 342699.May 4 2021, 4:43 AM
bsmith retitled this revision from [AArch64][SVE] Better utilisation of immediate forms for bitwise/arith intrinsics to [AArch64][SVE] Better utilisation of unpredicated forms of arithmetic intrinsics.
bsmith edited the summary of this revision. (Show Details)
  • Rework approach, include only arithmetic intrinsics, bitwise will be moved to new patch.
  • Include support for floating point arithmetic intrinsics.
  • Include support for wide shifts.
  • Call ISelLowering isAllActivePredicate from ISelDAGToDAG.
bsmith updated this revision to Diff 342985.May 5 2021, 3:52 AM
  • Fix clang format issues
peterwaller-arm added inline comments.May 6 2021, 3:01 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13868

Potential surprise: because it's a ref, updating Op is going to update N. So, why not just update N in the first place, or avoid using a reference? Might be surprising behaviour - I'd expect that if you're using an extra variable, it's to preserve N's original value, which isn't happening here.

sdesmalen added inline comments.May 6 2021, 3:26 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13868

or just make it a const SDValue &Op ?

17658

What is the value of implementing isAllActivePredicate in a separate, local function?

peterwaller-arm added inline comments.May 6 2021, 3:31 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13868

Not sure I understand, If it were a const ref wouldn't that disallow updating it with Op = Op.getOperand(0);?

sdesmalen added inline comments.May 6 2021, 3:42 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13868

<insert facepalm here> you're right, for a second there I got confused with pointers, where the first const applies to the object, not the pointer itself. For references, that's different.

bsmith added inline comments.May 6 2021, 3:47 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17658

The issue here is that convertMergedOpToPredOp also needs to be able to use isAllActivePredicate, but convertMergedOpToPredOp is also local as well as things further up the call tree (and I couldn't find a sensible way to access AArch64TargetLowering from the local function, given the available state, as well as that just feeling wrong), a sensible alternative to having this 'wrapper' function would be to make performIntrinsicCombine a member function, which is fine, I just wanted to avoid changing too much when there is no precedent.

sdesmalen added inline comments.May 6 2021, 5:27 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17658

Fair enough, thanks for explaining!

bsmith updated this revision to Diff 343416.May 6 2021, 8:22 AM
bsmith marked 2 inline comments as done.
  • Cleanup isAllActivePredicate to remove unneeded reference usage
sdesmalen accepted this revision.May 7 2021, 8:06 AM

LGTM!

This revision is now accepted and ready to land.May 7 2021, 8:06 AM
This revision was landed with ongoing or failed builds.May 10 2021, 5:05 AM
This revision was automatically updated to reflect the committed changes.