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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:561
+  Value *LHS, *RHS;
+  if (match(Vec, m_OneUse(m_BinOp(m_Value(LHS), m_Value(RHS)))) &&
+      (isSplatValue(LHS) || isSplatValue(RHS))) {
----------------
david-arm wrote:
> Is this always guaranteed to be safe? Could one of the lanes have previously generated an exception that we have now dropped?
@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.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:572
+          BinaryOperator::Create(OpC, NewII, SplatVal, Vec->getName(), &II);
+    } else if (auto *SplatVal = getSplatValue(LHS)) {
+      NewII->setArgOperand(1, RHS);
----------------
Should this just be an `else {`? as by this point `getSplatValue(LHS)` should be producing a result that is known safe to use.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-lasta-lastb.ll:186
+  %splat = shufflevector <vscale x 16 x i8> %splat_insert, <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer
+  %binop = udiv <vscale x 16 x i8> %splat, %vector
+  %last = tail call i8 @llvm.aarch64.sve.lastb.nxv16i8(<vscale x 16 x i1> %pg, <vscale x 16 x i8> %binop)
----------------
david-arm wrote:
> It might be worth having a FP binary operator test too here, since the `m_BinOp` match covers those as well.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106932/new/

https://reviews.llvm.org/D106932