[PATCH] D84201: [LangRef] Add integer wrapping operand bundles.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D84201#2165268 <https://reviews.llvm.org/D84201#2165268>, @efriedma wrote:

> 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.



================
Comment at: llvm/docs/LangRef.rst:2296
+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>`). The ``nuw`` and ``nsw`` bundles are only
----------------
efriedma wrote:
> > 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84201/new/

https://reviews.llvm.org/D84201