[PATCH] D116812: [AArch64][SVE][VLS] Move extends into arguments of comparisons

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Looks reasonable to me. A suggestion inline.

One other request, please could we have a brief comment or lispy expression on the function which does the combine to indicate why it exists / what the intent is.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15325
+                              SelectionDAG &DAG) {
+  assert(N->getOperand(0)->getOpcode() == ISD::SETCC);
+  const SDValue SetCC = N->getOperand(0);
----------------
I see you've protected `N->getOperand(0)` with this assert, but what about `N->getOpcode()`?

I would like to see a structure that looks a bit more like `performORCombine`, for example which looks like the below. That would pull the conditions into this function and make this assert into a `if (!...) return SDValue()` instead.

The motivation behind my ask is to put all the logic for this combine in this function.

```
static SDValue performORCombine(SDNode *N, TargetLowering::DAGCombinerInfo &DCI,
                                const AArch64Subtarget *Subtarget) {
  //...
  if (SDValue Res = tryCombineToEXTR(N, DCI))
    return Res;
...
}

static SDValue tryCombineToEXTR(SDNode *N,
                                TargetLowering::DAGCombinerInfo &DCI) {
  assert(N->getOpcode() == ISD::OR && "Unexpected root");
  // ...
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116812