[PATCH] D119424: [AArch64][SVE] Invert VSelect operand order and condition for predicated arithmetic operations

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17199-17200
 
+// (vselect (setcc (a) (0)) (a) (op (a) (b)))
+//  => (vselect (not setcc (a) (0)) (op (a) (b)) (a))
+static Optional<SDValue> tryInvertVSelectWithSetCC(SDNode *N,
----------------
I don't think this pattern should be sensitive to the contents of the setcc, so we don't need to be looking for a `0` nor an `0`.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17212-17213
+    return None;
+  if (SetCCOp0 == NOp2)
+    return None;
+
----------------
Per above, I don't think this should read operands of the setcc, except for inverting the condcode at the end.

The combine condition wants to be something which matches when `op` appears on the right, and `op`'s left operand is equal to the left operand of the vselect. This naturally prevents an infinite loop because it's not possible for the `VSelect.LHS == VSelect.RHS.LHS` to be true before and after the swap; and it's not true of `VSelect.LHS == VSelect.RHS`. (For these things to be true there would have to be a cycle of values, which is not allowed in the IR DAG).

I might find this a bit easier to read with LHS/RHS naming convention, since the vselect has an op0 which is the condcode, so its `Op1` is the LHS of the vselect, whereas the `OpOp0` is the LHS for the `op`.

So my suggestions for some clearer naming, if you need those things:
`NOp1` => `SelectA`
`NOp2` => `SelectB`
`OpOp0` => `OpLHS`

Then the condition to perform the combine is `SelectA == OpLHS`, if `SelectB.Opcode` could profit from the transformation.


================
Comment at: llvm/test/CodeGen/AArch64/sve-select.ll:654
+
+declare <vscale x 4 x float> @llvm.vp.fadd(<vscale x 4 x float>, <vscale x 4 x float>, <vscale x 4 x i1>, i32)
----------------
Extraneous?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119424