This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE][InstCombine] Move last{a,b} before binop if one operand is a splat value
ClosedPublic

Authored by mnadeem on Jul 27 2021, 9:29 PM.

Details

Summary

Move the last{a,b} operation to the vector operand of the binary instruction if
the binop's operand is a splat value. This essentially converts the binop
to a scalar operation.

Example:

// If x and/or y is a splat value:
lastX (binop (x, y)) --> binop(lastX(x), lastX(y))

Diff Detail

Event Timeline

mnadeem created this revision.Jul 27 2021, 9:29 PM
mnadeem requested review of this revision.Jul 27 2021, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2021, 9:29 PM

Seeing this pattern internally, looking for comments about whether this is relevant to the community.

david-arm added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
561

Is this always guaranteed to be safe? Could one of the lanes have previously generated an exception that we have now dropped?

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

It might be worth having a FP binary operator test too here, since the m_BinOp match covers those as well.

Matt added a subscriber: Matt.Jul 28 2021, 2:48 PM
paulwalker-arm added inline comments.Jul 29 2021, 9:50 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
561

@david-arm I don't believe there's any need to worry about exceptions here as LLVM BinOps are defined to have no side effects.

572

Should this just be an else {? as by this point getSplatValue(LHS) should be producing a result that is known safe to use.

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

I agree as looking at the code above I think these will highlight that the transform is going to unnecessarily drop fastmath flags. The same is possibly also true for things like sdiv's exact flag.

mnadeem updated this revision to Diff 363592.Aug 2 2021, 3:55 PM
mnadeem edited the summary of this revision. (Show Details)
  • Updated the code to make sure that the binary operator's flags are copied as well.
  • Added more tests to check that the math flags are also copied.
  • Added tests for add and fadd.
mnadeem marked 3 inline comments as done.Aug 2 2021, 3:58 PM
mnadeem added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
572

Removed the use of getSplatValue() altogether.

paulwalker-arm added inline comments.Aug 3 2021, 7:54 AM
llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-lasta-lastb.ll
278–348

Given you have div and div tests with and without flags, do these add/fadd tests testing anything new? If not then there's not much point keeping them.

278–348

Given you have div and fdiv tests with and without flags, do these add/fadd tests testing anything new? If not then there's not much point keeping them.

350

I was going to group this test with the above comment but looking at the output there is no transformation. Is this intended? I would have expected a scalar add.

Sorry forgot to ask if you've used update_test_checks.py to generate the CHECK lines within sve-intrinsic-opts-lasta-lastb.ll like the other tests within the file. If not then please do so as otherwise the next person along will have unnecessary changes in their patch.

mnadeem updated this revision to Diff 364670.Aug 5 2021, 6:34 PM

Fix formatting, remove/update tests according to comments.

mnadeem marked an inline comment as done.Aug 5 2021, 6:39 PM
mnadeem added inline comments.
llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-lasta-lastb.ll
350

Removed that test as it was not relevant to this patch. The shuffle was pushed after the add by some other transformation.

mnadeem marked an inline comment as done.Aug 5 2021, 6:39 PM
paulwalker-arm accepted this revision.Aug 6 2021, 2:43 AM
paulwalker-arm added inline comments.
llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-lasta-lastb.ll
287

don't

This revision is now accepted and ready to land.Aug 6 2021, 2:43 AM