[PATCH] D144379: [AArch64] Fix abs(sub nsw) -> absd

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:802
+  virtual bool preferABDSToABSWithNSW(EVT VT) const {
+    return isOperationLegalOrCustom(ISD::ABDS, VT);
+  }
----------------
just return true by default - isOperationLegalOrCustom can cause problems after custom lowering if it re-expands the ABDS pattern


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10525
     if (AbsOp1->getFlags().hasNoSignedWrap() &&
-        TLI.isOperationLegal(ISD::ABDS, VT)) {
+        TLI.preferABDSToABSWithNSW(VT)) {
       SDValue ABD = DAG.getNode(ISD::ABDS, DL, VT, Op0, Op1);
----------------
It would be better to use hasOperation() && TLI.preferABDSToABSWithNSW and remove the Legal/Custom checks from the callback - otherwise you can potentially end up in loops where the custom lowering re-expands the abds pattern and its gets matched over and over again....


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:57087
+bool X86TargetLowering::preferABDSToABSWithNSW(EVT VT) const {
+  return isOperationLegal(ISD::ABDS, VT);
+}
----------------
just return false - x86 doesn't have any ABDS instructions


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

https://reviews.llvm.org/D144379