Page MenuHomePhabricator

[LangRef] Element-wise Integral Reduction Intrinsics
ClosedPublic

Authored by lebedev.ri on Jun 15 2020, 1:42 AM.

Details

Summary

This is the proposed wording for the [RFC] Integer Intrinsics for abs, in unsigned/signed min/max.

Link to RFC:
https://lists.llvm.org/pipermail/llvm-dev/2020-June/142257.html

Proposed alive2 implementation: https://github.com/AliveToolkit/alive2/pull/353

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 15 2020, 1:42 AM
lebedev.ri edited the summary of this revision. (Show Details)Jun 15 2020, 1:45 AM
nikic added a reviewer: nikic.Jun 15 2020, 1:53 AM
nikic added a subscriber: nikic.

I find the naming of these intrinsics somewhat unintuitive. My first association with llvm.reduce.smax is that this is the non-experimental version of llvm.experimental.vector.reduce.smax, because reduction (to me) carries a strong vector reduction connotation. I can still see how these might be "reductions" for min/max (in which case I would expect the intrinsics to be variadic though), but not seeing the analogy at all for abs().

I would suggest to instead call these just llvm.smax, llvm.abs etc, which seems more in line with existing integer intrinsic names.

yeah, “reduce” should be dropped

xbolva00 added inline comments.Jun 15 2020, 2:12 AM
llvm/docs/LangRef.rst
15528

What abouts nabs?

We pattern match it currently too.

Per popular demand, drop .reduce. from intrinsic names.

spatel added inline comments.Jun 15 2020, 11:02 AM
llvm/docs/LangRef.rst
15402–15408

We should avoid "reduction" vocabulary with any of these operations since that term is already used for vector ops.

abs() belongs in the "Standard C Library Intrinsics" section that starts around line 12099.
If we relax that title to "Standard C/C++ Library Intrinsics", then min/max can live there too?

15428–15429

Return the larger of a and b comparing the values as signed integers.
Vector intrinsics operate on a per-element basis. The larger element of a and b at a given index is returned for that index.

15436

I'd remove the "return type matches..." line above and work that clause in here:
"The argument types must match each other, and the return type must match the argument type."

15457

Return the smaller of a and b comparing the values as signed integers.
Vector intrinsics operate on a per-element basis. The smaller element of a and b at a given index is returned for that index.

15486–15488

Return the larger of a and b comparing the values as unsigned integers.
Vector intrinsics operate on a per-element basis. The larger element of a and b at a given index is returned for that index.

15515–15516

Return the smaller of a and b comparing the values as unsigned integers.
Vector intrinsics operate on a per-element basis. The smaller element of a and b at a given index is returned for that index.

15538–15539

Do we need to include the poison option?

The SDAG node only has:

/// Note: A value of INT_MIN will return INT_MIN, no saturation or overflow
/// is performed.
nikic added inline comments.Jun 15 2020, 1:14 PM
llvm/docs/LangRef.rst
15528

We do, but it's probably much less common than abs() and doesn't have any backend support. I think we can get away with representing it as 0 - abs(x) (or just the same way we do now).

15538–15539

The poison option is the equivalent of the sub nsw flag that was used previously. It doesn't matter for lowering, but it is used for analysis sometimes, in particular we can only assume that the llvm.abs() result is actually positive if the poison flag is set.

15556

Missing space before "if".

lebedev.ri marked 13 inline comments as done.

@spatel @nikic @xbolva00 thank you for taking a look!
Addressing review notes.

lebedev.ri added inline comments.
llvm/docs/LangRef.rst
15528

We do indeed have SPF_NABS, but i strongly believe that is again because we have to, not because it's useful.
Given, -(a < 0 ? -a : a), currently, we can't possibly preserve that pattern from sinking negation into it.
And as soon as it's sunk, it is no longer an ABS, and we've lost track of it.
If we have an intrinsic, that will no longer be an issue.
I currently see no need for nabs intinsic.

15538–15539

The poison option is the equivalent of the sub nsw flag that was used previously.

Precisely, yes. This intrinsic will be pretty powerless without that flag.

simoll added a subscriber: simoll.Jun 16 2020, 6:05 AM
simoll added inline comments.Jun 16 2020, 6:09 AM
llvm/docs/LangRef.rst
15538–15539

Couldn't we add integer overflow flags to CallInsts the same way we have fast math flags? That should make this extra argument redundant.
Also, this would allow us to have nsw, nuw on VP intrinsics (https://llvm.org/docs/LangRef.html#id2355) and other integer intrinsics.

spatel accepted this revision.Jun 16 2020, 6:45 AM

LGTM - see inline for grammar nits. Please remove "Reduction" from the title of this review too.

But we should wait at least a week before pushing in case there is more feedback here or on llvm-dev.

llvm/docs/LangRef.rst
12277

Remove comma after "width".

12294

Remove comma after "type".

12298

typo: a a

12305–12306

First line is a bit awkward to me. How about:
"Returns the magnitude (always positive) of the argument or each element of a vector argument."

12317

Remove comma after "width".

12334

Remove comma after "type".

12346

Remove comma after "width".

12363

Remove comma after "type".

12375

Remove comma after "width".

12392

Remove comma after "type".

12404

Remove comma after "width".

12421

Remove comma after "type".

This revision is now accepted and ready to land.Jun 16 2020, 6:45 AM
lebedev.ri marked 14 inline comments as done.

@spatel thank you for taking a look!
Addressing various typo nits.

llvm/docs/LangRef.rst
15538–15539

Possibly, but that seems like a more general rework that doesn't have to block this.

nikic accepted this revision.Jun 16 2020, 12:48 PM

LGTM as well.

llvm/docs/LangRef.rst
12287

nit: "returns *the* absolute value"

12305–12306

The (always positive) note seems a bit odd here, given the INT_MIN behavior.

This LGTM, but it shouldn't be committed without an actual implementation.

This LGTM,

but it shouldn't be committed without an actual implementation.

I'm going to assume that by implementation only the actual addition of the intrinsics is meant,
not any/all of the piping/teaching about the new intrinsics.

nikic added a comment.Jun 19 2020, 9:04 AM

@lebedev.ri As we already have the corresponding SDAG nodes, I would expect the initial implementation to just add the intrinsics, the SDAG builder code, and a cursory X86 test that the intrinsics get lowered.

nikic added a comment.Jun 27 2020, 1:52 AM

As there hasn't been any further feedback on llvm-dev, time to move forward with this?

+1, but initial implementation should be done first, as noted here:

This LGTM, but it shouldn't be committed without an actual implementation.

This revision was automatically updated to reflect the committed changes.