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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer 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);
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142288