[PATCH] D29639: [SelectionDAG] Add a signed integer absolute ISD node

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

I think this is a good idea, in terms of the usual trade-off (we lose the opportunity to combine the xor with other logic, simplifying it, etc.) Especially since we only introduce this post-legalization.
But I'd like to hear what other people think too.



================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:343
+    /// ABS - Determine the unsigned absolute value of a signed integer value.
+    /// Note: A value of INT_MIN will return INT_MIN, no saturation or overflow
+    /// is performed.
----------------
Is this the right behavior for a target-independent intrinsic? That is, do the other (non-x86) platforms that have an ABS instruction have the same behavior?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5502
+  if (DAG.isConstantIntBuildVectorOrConstantInt(N0))
+    return DAG.getNode(ISD::ABS, SDLoc(N), VT, N0);
+  // fold (abs (abs x)) -> (abs x)
----------------
:-(
(I was really surprised by this, but looks like we have this pattern everywhere.)


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:20853
+  return DAG.getNode(ISD::CONCAT_VECTORS, dl, VT,
+                     DAG.getNode(Op.getOpcode(), dl, NewVT, Lo),
+                     DAG.getNode(Op.getOpcode(), dl, NewVT, Hi));
----------------
Is there a reason not to use ISD::ABS explicitly here? I think it'd be clearer.


Repository:
  rL LLVM

https://reviews.llvm.org/D29639