Ping.

Incorporate changes and text requested by Andrew Kaylor. Remove mention of the lack of barriers and include text on how to accomplish mixing without barriers.

Address review comment. Mention the possibility of the restrictions mentioned here changing in the future, but don't make any promises.

In D67360#1667091, @uweigand wrote:

My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?

Now, I guess it still makes sense to document the restriction even if it is temporary, I'm just wondering whether this should be made explicit in the docs.

Well, I like it. The problem is solved, the pseudocode is I find a little easier to read, and the System/Z instruction sequence looks like a sideways move performance-wise.

In D67105#1657765, @uweigand wrote:

Ah, good point. Currently, constrained floating-point intrinsics are never constant-folded. In general, this would likely be wrong anyway, since we might not know the current rounding mode that applies.

However, in the specific case of a fptoui, we don't depend on the current rounding mode (it's always round-to-zero even for the constrained case), so we actually could constant fold those in the cases where we the result is in range.

@kpn -- the test cases you added specifically verify that constrained fptoui does not constant-fold but does the runtime conversion. Should those be changed to use nonconstant arguments then?

Address review comments.

Tweak documentation hopefully as requested. Remove a stray blank line that somehow got in there.

Ping

I don't believe I have any further comments. What do the front-end guys say?

John already gave his go-ahead, and I'm adding my LGTM.

LGTM

Does this work for anything that uses TreeTransform, like C++ templates?

What will this do to software that does frequently use NaNs and doesn't treat them as exceptional?

Address review comments.

In D65994#1622840, @aaron.ballman wrote:

In general, this seems reasonable, but is missing test code.

Address review comments. Rebase.

In D65226#1615137, @uweigand wrote:

Ping?

Should we move ahead with this? I believe this is still a pre-req for D63782 ...

I actually don't have much of an opinion on what the command line argument form should be. It may be helpful for it to be the same as one of the commonly deployed compilers. The worst I think would be pretty close but with subtle differences. So if it can be made to work like Intel's compiler I'm fine with that. But I'm hoping that more people in the community chime in since having a consensus would be best. Personally, I'm not yet giving any final sign-offs to tickets since I don't think I've been here long enough.

So the PowerPC code regressions will be fixed once the strict tickets make it into the tree it sounds like.

In D62731#1601310, @mibintc wrote:

I think it would be convenient to have an "unset" setting for the different constrained modes, otherwise you need a boolean that says "no value was provided for this option". But i'm a frontend person so I may need to have my attitude adjusted.

It looks like we're unrolling some vectors where before we weren't. That seems unfortunate. Is that the reason for the generated code quality regressions?

Address review comments.

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.