[PATCH] D142288: [X86] Add basic vector handling for ISD::ABDS/ABDU (absolute difference) nodes

[Ricardo Jesus via Phabricator via llvm-commits]

rjj added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10193
     if (AbsOp1->getFlags().hasNoSignedWrap() &&
-        TLI.isOperationLegalOrCustom(ISD::ABDS, VT))
-      return DAG.getNode(ISD::ABDS, SDLoc(N), VT, Op0, Op1);
+        TLI.isOperationLegal(ISD::ABDS, VT)) {
+      SDValue ABD = DAG.getNode(ISD::ABDS, DL, VT, Op0, Op1);
----------------
SjoerdMeijer wrote:
> RKSimon wrote:
> > SjoerdMeijer wrote:
> > > We are seeing an AArch64 regression because of this patch, and the `isOperationLegalOrCustom` -> `isOperationLegal` change seems to be causing this. We are no longer generating a absolute difference and accumulate instruction, which apparently hinges on this ABDS rewrite first. Looks like all sorts of tests are missing for this, but we will add them.
> > > 
> > > I don't quite understand this change, why "LegalOrCustom" would not be good enough and how that relates to NSW flags. Any insights would be appreciated. I was also surprised to see:
> > > 
> > >     setOperationAction(ISD::ABDS,             VT, Custom);
> > > 
> > > in this patch. I.e., the "Custom" surprised me and how that works with the `isOperationLegal` check.
> > > Again, any tips would be welcome while I look a bit more into this. :)
> > Its a fudge to get around the fact that expansion from ABDS will be at least 3 ops or more - sub(smax(x,y),smin(x,y)) / selectcc(sgt(x,y),sub(x,y),sub(y,x)) / etc. - but if we have an abs instruction then abs(sub_nsw(x,y)) will be better in this case.
> > 
> > A possible (untested) alternative could be:
> > ```
> > (TLI.isOperationLegal(ISD::ABDS, VT) || !TLI.isOperationLegalOrCustom(ISD::ABS, VT))
> > ```
> Thanks, we will be looking into this!
@RKSimon I've tested your suggestion and it still doesn't get us `abs(sub nsw) -> abds` back in AArch64. I still don't understand the `isOperationLegalOrCustom -> isOperationLegal` change. Could you please give us a few more pointers please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142288