Thanks for the fast review!

In D63782#1582778, @cameron.mcinally wrote:

In D63782#1579595, @efriedma wrote:

How aggressive is LLVM's UB handling? Would it remove an entire block/function if UB is found in it?

If LLVM can prove a basic block unconditionally executes UB, it will be erased. But "unconditionally" is an important qualifier. For example, consider the following function: void f(void g()) { g(); *(int*)0 = 0; }. The call to g isn't erased because we can't prove g will return.

The constrained FP intrinsics should be opaque enough (besides 'ignore'), so that sounds fine to me.

Seems like we should let invalid fptoXi's return poison, like the unconstrained versions do. Anyone see a problem with this?

Stupid question: what's a "global barrier"?

In D63782#1561585, @andrew.w.kaylor wrote:

I might be opening a can of worms here and I'm not a language expert, but it isn't clear to me from reading the C99 standard that defining fptosi/fptoui as returning poison values in the unrepresentable case allows correct implementation of the C standard. That is, it doesn't seem to me that the standard actually says this is undefined behavior. It just says the resulting value is unspecified, and the exception behavior is explicitly defined. On the other hand, C++ does say clearly that it is undefined behavior, right?

I understand that in the unconstrained case LLVM doesn't care about FP exceptions and that we would like the LLVM IR definition to be more precise than the C standard. I'm just trying to get my head wrapped around why we're doing what we are in that case and what we need to do to correctly implement strict FP semantics.

If we say that the constrained version returns undef in the unrepresentable case and clearly emphasize how this differs from the standard fptosi/fptoui instructions, would that have the effect of keeping the operation around so that it can raise the exception while still giving us well-defined IR semantics?

I was convinced in email that having the constrained intrinsics take fast math flags could be useful in some circumstances. The example that was given was that perhaps FMA contraction could be requested. So, when generating constrained intrinsics the fast math flags and the FP metadata is now handled exactly the same as in non-constrained mode. I've updated the documentation to warn about that when turning constrained mode on and off.

Address review comments. Eliminate ConstrainedFPIntrinsic's ebInvalid and rmInvalid enumeration values and replace them with use of the Optional<> class. Adjust the rest of the patch to take that into account. ConstrainedFPIntrinsic now supports string <--> enum conversions.

In D63782#1560156, @cameron.mcinally wrote:

In D63782#1560136, @craig.topper wrote:

In D63782#1560123, @cameron.mcinally wrote:

So pragmatically, an invalid exception is an alarm that the code is off track. As long as the exception is handled appropriately (default or an alternative), the result of the invalid operation shouldn't matter. Whatever LLVM wants to do with the value gets no arguments from me, since we've already self-destructed (unless the program handles the exception gracefully, but that wouldn't require a defined result from the invalid operation anyway).

But the exception could be masked couldn't it?

Yeah, but I'm not sure if it matters. The program has already failed, so there's no guarantee that the results are useful.

Our typical user enables traps (inv, divz, and ovf) during development to convince themselves that the code is safe. Production runs are then done with traps disabled. But, of course, traps may be reenabled if a runtime problem is later found. They're basically a sanity check.

In D63782#1558430, @efriedma wrote:

What happens if the input float is out of range? fptosi/fptoui instructions produce poison; not sure if you want that here.

Replaced by D63782.

I found out that some compilers, including SAS/C, will warn about enum overlaps like the one here under discussion. So I now believe John is correct and that eliminating that overlap is the right thing to do. Over the weekend I'll be thinking about whether or not to try and find an alternative, but I'll be out of cell phone range until late Sunday evening.

Andy, can I get you to chime in?

In D43515#1547200, @andrew.w.kaylor wrote:

How would you feel about rebooting this as a new patch? There's a lot of irrelevant history here, and I feel like I'm missing some context as I review it.

Add static methods to convert between a StringRef and the enums for RoundingMode or ExceptionBehavior.

Address review comments.

Ping

Ping.

Ping.

I don't think this needs to be open anymore.

Address the rest of the review comments, hopefully.

Oh, this ticket is not going to die from neglect. It is true that D43515 is a higher priority, but I need this IRBuilder work done as well. My department head wanted it done by the end of _last_ year. It's not going to die.

I wanted to get the API straight before working on documentation.

Incorporating feedback from D53157 is probably a good idea. Looks like that hasn't been done yet here completely.

Address review comments.

Address review comments from here and from D55897.

Drew is working with me here at SAS Institute, Inc.

I'm waiting for a signoff from at least one front-end guy. I hope the mode setting that allows us to keep front ends from having to touch every use of the IRBuilder is something that works for clang. But I haven't heard anything from @rsmith or any other front-end person.

The only thing left in this ticket to commit is the fptosi and fptoui changes. I'm working on incorporating review comments from D55897 into this ticket and fixing other bugs that I'm coming across. I'll hopefully update this ticket next week.

If I recall, fneg is not a constrained intrinsic because an fneg will never itself trap.

Is this an allowed transform?

Excellent. Thank you for all the reviews! I will commit this on Monday morning so I can keep an eye on the bots.

Address review comments.

Why does Verifier::visitIntrinsicCall() not choke when given an intrinsic it doesn't know about?

Address review comments: Add a comment as requested. Remove bogus optimizations to let the base case work. Update tests.

In D61447#1490050, @cameron.mcinally wrote:

In D61447#1488502, @cameron.mcinally wrote:

@spatel I'm not a PatternMatch expert, but I think that change is what we wanted. That is, to continue to match FSub(-0.0, X) as an FNeg(X).

Just FYI that an equivalent patch was made to the PatternMatcher in D61520. I needed this for some FNeg constant folding work. Thanks for the inspirations, Kevin.

Address review comments.

Address review comments.

Would it make sense to also update docs/AddingConstrainedIntrinsics.rst please?