[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);
----------------
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))
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142288