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

[David Truby via Phabricator via llvm-commits]

DavidTruby added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15325
+                              SelectionDAG &DAG) {
+  assert(N->getOperand(0)->getOpcode() == ISD::SETCC);
+  const SDValue SetCC = N->getOperand(0);
----------------
peterwaller-arm wrote:
> 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");
>   // ...
> }
> ```
> 
I wrote it this way following some of the other functions in this file, it doesn't seem there's much consistency between this way and they way you're suggesting!

My gut instinct tells me it's clearer in this case to have the condition in the other function and the assertion here, because then it shows clearly in which circumstances you're going to call this combine. I'm happy to change it if you think it would be better the other way around though!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116812