Page MenuHomePhabricator

[SelectionDAG] Add a signed integer absolute ISD node
ClosedPublic

Authored by RKSimon on Feb 7 2017, 7:07 AM.

Details

Summary

Reduced version of D26357 - based on the discussion on llvm-dev about canonicalization of UMIN/UMAX/SMIN/SMAX as well as ABS I've reduced that patch to just the ABS ISD node (with x86/sse support) to improve basic combines and lowering.

AFAICT ARM/AArch64, PowerPC and NVPTX all have similar instructions so I'd like to make this a generic opcode and move us away from the hard coded tablegen patterns which makes it tricky to match more complex patterns.

At the moment this patch doesn't attempt legalization (and I only create an ABS node if its legal/custom) - I can add the legalization code from D26357 if people think it useful at this stage - it will let us do some extra combines, knownbits handling etc.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Feb 7 2017, 7:07 AM
mkuper edited edge metadata.Feb 15 2017, 2:23 PM

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.

include/llvm/CodeGen/ISDOpcodes.h
343 ↗(On Diff #87426)

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?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5502 ↗(On Diff #87426)

:-(
(I was really surprised by this, but looks like we have this pattern everywhere.)

lib/Target/X86/X86ISelLowering.cpp
20853 ↗(On Diff #87426)

Is there a reason not to use ISD::ABS explicitly here? I think it'd be clearer.

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

include/llvm/CodeGen/ISDOpcodes.h
343 ↗(On Diff #87426)

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.

lib/Target/X86/X86ISelLowering.cpp
20853 ↗(On Diff #87426)

That's me trying to be too generic - will fix.

RKSimon updated this revision to Diff 88721.Feb 16 2017, 5:43 AM

rebased and tidied up X86 LowerABS

@hfinkel @jmolloy Is this approach compatible with PowerPC and ARM? Cheers.

jmolloy edited edge metadata.Feb 21 2017, 6:43 AM
jmolloy added a subscriber: jgreenhalgh.

Hi,

Having asked around: The way we define this is that the VABS instruction takes a signed integer and outputs an unsigned integer, getting around this problem.

However, I believe the output of VABS(INT_MIN) is indeed bit-identical to INT_MIN.

+ @jgreenhalgh to confirm I haven't mangled his explanation.

James

Hi,

Having asked around: The way we define this is that the VABS instruction takes a signed integer and outputs an unsigned integer, getting around this problem.

However, I believe the output of VABS(INT_MIN) is indeed bit-identical to INT_MIN.

+ @jgreenhalgh to confirm I haven't mangled his explanation.

Only a minor tweak on a pedantic point - the instruction doesn't really have an idea of signed/unsigned - just bits. The effect is as if you were calculating in an unsigned value of the same number of bits as the input.

That doesn't hold for the intrinsics which are all signed -> signed. But the behaviour there is ABS(INT_MIN) -> INT_MIN, so the behaviour in this patch looks fine for ARM/AArch64 Advanced SIMD.

RKSimon updated this revision to Diff 89414.Feb 22 2017, 1:12 PM

Rebased and made it explicit in the ISDOpcodes.h that the input and result of ISD::ABS have the same bitwidth.

Does anyone have any other comments? What about legalization - should I add it as part of this patch or just keep to the legal cases for now?

Would it make sense to use ISD::ABS over combineIntegerAbs in X86ISelLowering.cpp?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5514 ↗(On Diff #89414)

There are a lot of other folds we could do here (e.g. abs(-x) -> abs(x)), but maybe not worth worrying about?

Would it make sense to use ISD::ABS over combineIntegerAbs in X86ISelLowering.cpp?

Yes, although probably as a followup once the legalization support is in place.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
5514 ↗(On Diff #89414)

Definitely scope for further combines, but I'd rather see them in the wild before adding them.

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

Hexagon has integer ABS for 32- and 64-bit integers, and for both of them abs(min_int) == min_int, so this works for us. FWIW, we also have a saturating abs for 32-bit integers.

efriedma accepted this revision.Mar 14 2017, 11:10 AM

LGTM.

It looks like we have consensus that this is generally useful, and the implementation looks fine.

This revision is now accepted and ready to land.Mar 14 2017, 11:10 AM
This revision was automatically updated to reflect the committed changes.