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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon 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);
----------------
rjj wrote:
> 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?
Well, I did say it was untested.... Short of adding a TLI call (preferABDToABS(EVT)?) I'm not sure what to do here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142288