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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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:
> 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
If this is for SVE then we might just be able to add tablegen patterns to cover it, providing they know that it is a nsw sub. It is a bit of a shame to need the extra patterns, so maybe preferABDToABS is better and then it can share the code.

Unfortunately SDAG can be a little awkward when there are multiple backends involved, where "Custom" means too many different things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142288