Page MenuHomePhabricator

[LangRef] Add integer wrapping operand bundles.
Needs ReviewPublic

Authored by fhahn on Mon, Jul 20, 2:04 PM.

Details

Summary

This patch introduces new nuw and nsw operand bundles, to indicate
no-unsigned-wrap and no-signed-wrap for the computation of the result of
a call.

Initially, functions that want to use it need to opt-in, as there are
many cases where the bundle probably makes little sense. This mirrors
fast-math flags, which can be specified directly for calls returning
floating point values. Adding nuw/nsw as operand bundles is very
lightweight. If there are a lot of users, it might make sense to
consider adding the flags directly to calls, but I think operand bundles
allow us to make progress quickly and see how widely useful this
actually is.

Tot start with, llvm.matrix.multiply will use it, but other candidates
are the integer reduction intrinsics.

Diff Detail

Unit TestsFailed

TimeTest
6,750 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,660 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

fhahn created this revision.Mon, Jul 20, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 20, 2:04 PM
fhahn updated this revision to Diff 279346.Mon, Jul 20, 2:13 PM

Add example.

I don't understand how this is easier than adding the flags to calls?

jdoerfert added inline comments.Mon, Jul 20, 10:34 PM
llvm/docs/LangRef.rst
2306

[Drive By]

Should we add a reference to nuw/nsw specification (if we have one)?

I'm not sure the the description is explicit enough. `if
unsigned and/or signed overflow, respectively, occurs`, where/when?

Nit: I would say the type is spelled i1 but I don't care much.

fhahn updated this revision to Diff 279507.Tue, Jul 21, 6:44 AM
fhahn marked an inline comment as done.

Improve wording.

I don't understand how this is easier than adding the flags to calls?

The main advantages IMO of the bundle approach IMO are

  1. no need to modify the IR directly, which might be quite controversial
  2. adding the flags directly to calls means all calls need to pay some cost (a few extra bits somewhere)
  3. the integer wrapping flags are probably much less useful for calls than the fast-math flags. The fast-math flags describe a property of the result value (e.g does not contain signed zeroes) which is useful to know when using the result of the call, whereas the wrapping flags mostly describe how the result is computed (without overflow), which is not too helpful to know about at the call site, but helpful when lowering certain intrinsics.

Hence I think the bundles are more lightweight than adding the flags directly to start with. If they prove useful for a large range of calls, it should be easy to auto-upgrade the existing uses to flags on the instruction and we would have a solid data point to argue for adding them directly to calls. Does that make sense?

llvm/docs/LangRef.rst
2306

Should we add a reference to nuw/nsw specification (if we have one)?

I don't think there is. AFAIK it is only specified in the semantics of the instructions taking nuw/nsw flags.

I'm not sure the the description is explicit enough. if unsigned and/or signed overflow, respectively, occurs, where/when?

yeah, I should have been more explicit. I changed it to

the return value of the called function is a :ref:poison value <poisonvalues> if unsigned and/or signed overflow, respectively, occurs during any computation the return value depends on in the called function (see value *dependence* as defined for :ref:poison value <poisonvalues>).

Nit: I would say the type is spelled i1 but I don't care much.

  1. adding the flags directly to calls means all calls need to pay some cost (a few extra bits somewhere)

But calls already have fast math flags; I assume these would alias the same bits

fhahn added a comment.Tue, Jul 21, 7:31 AM
  1. adding the flags directly to calls means all calls need to pay some cost (a few extra bits somewhere)

But calls already have fast math flags; I assume these would alias the same bits

Oh right, I guess we could do that and then it would be free in terms of size. But I think the main question is if nuw/nsw flags are best use for those bits and what makes them special, as compared to other properties for which we have bundles or call site attributes (mainly point 3) about the general usefulness of those flags.

SjoerdMeijer added a subscriber: efriedma.

Interesting work. Adding @efriedma, with whom I recently had a overflow discussion for intrinsic @llvm.get.active.lane.mask, and I think that this would help me.

nikic added a subscriber: simoll.Tue, Jul 21, 1:19 PM

I guess there are sort of three two precedents in IR for this sort of thing:

  1. fast-math flags, as mentioned earlier.
  2. constant arguments; see, for example, llvm.ctlz
  3. metadata; for example, !range on calls.

I don't think we've really ever used operand bundles this way.

Metadata might make more sense here.

llvm/docs/LangRef.rst
2296

if unsigned and/or signed overflow, respectively, occurs during any computation the return value depends on in the called function

I don't think a generic definition here is really useful; the semantics are really specific to the intrinsic in question.

In particular, for matrix multiplication, this definition is a but unclear. The order of the addition operations isn't defined for matrix multiplication; that matters for nsw.

fhahn marked an inline comment as done.Tue, Jul 21, 3:02 PM

I guess there are sort of three two precedents in IR for this sort of thing:

  1. fast-math flags, as mentioned earlier.
  2. constant arguments; see, for example, llvm.ctlz
  3. metadata; for example, !range on calls.

I don't think we've really ever used operand bundles this way.

Metadata might make more sense here.

There are too many things to do the same thing :)

My understanding from the langref is that operand bundles basically "are like metadata, but dropping them is incorrect and will change program semantics". I guess we might actually need to drop the metadata on some changes to the call. I don't really mind whichever solution to pursue, they should all achieve the same result. So unless there are any concerns for using metadata, I'll update the patch in a day or two.

llvm/docs/LangRef.rst
2296

Agreed, it probably makes most sense to delegate that to the intrinsics that opted in. Also, it should probably be limited to intrinsics, not sure it makes sense for arbitrary functions.