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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Functionally I think it's looking reasonable to me. A few more stylistic nits.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17201
+//  => (vselect (not setcc (a) (n)) (op (a) (b)) (a))
+static Optional<SDValue> tryInvertVSelectWithSetCC(SDNode *N,
+                                                   SelectionDAG &DAG) {
----------------
SDValue has Optional-like semantics built in: an `SDValue()` evaluates to false, so the optional is unnecessary here (I didn't see any other cases of Optional being used as a return argument like this in this file).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17208-17209
+  SDValue SetCC = N->getOperand(0);
+  if (SetCC.getOpcode() != ISD::SETCC || !NTy.isScalableVector() ||
+      !SetCC.hasOneUse())
+    return None;
----------------
This if statement has a mix of conditions, referring to different things. It would be slightly better if it were grouped so that the setcc ones are next to each other at least. Better still, I might hoist the scalable query up to the top:
```
auto NTy = N->getValueType(0);
if (!NTy.isScalableVector())
  return None;
```
My concern is that the condition is hiding in there, someone scanning their eyes vertically at the condition might see 'setcc, setcc' on adjacent lines, and think that all of the conditions relate to setcc, where they do not.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17226
+  auto InverseSetCC = DAG.getSetCC(
+      SDLoc(SetCC), SetCC.getValueType(), SetCCOp0, SetCC.getOperand(1),
+      ISD::getSetCCInverse(CC, SetCC.getValueType()));
----------------
`SetCCOp0` is named here but the `SetCC.getOperand(1)` is not assigned a variable, so I'd drop the variable in this case because it is both single use and doesn't add any extra information.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17239-17240
 static SDValue performVSelectCombine(SDNode *N, SelectionDAG &DAG) {
+  if (auto InvertResult = tryInvertVSelectWithSetCC(N, DAG))
+    return InvertResult.getValue();
+
----------------
Name clarity: inverting the `vselect` sounds like `(not vselect)`. The object being inverted is the setcc condition code. Suggestion: A better name might be `trySwapVSelectOperands`?


================
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,
----------------
peterwaller-arm wrote:
> 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`.
> 
Just checking if you saw the suggestion above this comment, which adds rationale and makes the select pattern comment a little easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119424