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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Thanks Michael - does anyone from a non-X86 target have any comments?



================
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.
----------------
mkuper wrote:
> 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?
AFAICT this is the norm and matches what we do for constant folding of the std::abs() function as well, although technically its undefined if the result isn't representable.

ARM NEON has the same functionality with its basic vabs_* intrinsic, and has a saturating version vqabs_* as well.
PowerPC has vec_abs / vec_abss.
Not sure about the other targets?

Saturating patterns can probably be best be handled by abs(smax(v, INT_MIN + 1)) style canonicalizations? But that would still have to be target specific.


================
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));
----------------
mkuper wrote:
> Is there a reason not to use ISD::ABS explicitly here? I think it'd be clearer.
That's me trying to be too generic - will fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D29639