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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm accepted this revision.
peterwaller-arm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15319
+  return OC == ISD::LOAD || OC == ISD::MLOAD ||
+         ISD::isConstantSplatVectorAllZeros(N.getNode());
+}
----------------
I see tests hitting the `MLOAD` case, but not the `LOAD` nor `isConstantSplatVectorAllZeros` cases. Any thoughts on that?

Another idea that pops into mind: I might expect that all constants could be cheap to extend, as logically they can be rewritten as another constant. I see that `FoldConstantArithmetic` might achieve this in `getNode` for the `SIGN_EXTEND`. Not necessarily for this patch unless you agree and think it's easy to do.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15325
+                              SelectionDAG &DAG) {
+  assert(N->getOperand(0)->getOpcode() == ISD::SETCC);
+  const SDValue SetCC = N->getOperand(0);
----------------
DavidTruby wrote:
> 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!
I agree there are different ways this is written, the above comment is only a suggestion so I'm happy to proceed as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116812