Rebase. Ping.

In D55506#1408435, @wuzish wrote:

I think we can have a flag to indicate rounding mode for float operator such as FADD instead of STRICT_FADD, because STRICT_FADD does have side effect(chain), but some target (eg. RISCV) has static rounding mode encoded in the instruction which does not side effect. Then we can optimize specific STRICT_FADD node which ignoring exception into normal FADD with corresponding static round mode.

Address review comments.

Without this patch is the fneg instruction getting turned into a subtraction instruction? I hope not on any platform!

I'm seeing a regression in WebAssembly/PR40267.ll that I need to look into. If anyone else is seeing this failure let me know.

I emailed llvm-dev and cfe-dev on January 16th. The only responses I got back were of the don't care variety. For now it seems the constrained intrinsics will only be used by clang.

Address review comments.

Add back a rounding mode argument to constrained fptrunc as requested by Connor Abbott.

OK, I'm working on it now.

In D52839#1263794, @rsmith wrote:

It was pointed out before I split this ticket out that I wasn't properly passing the correct operand in ExpandNode() when handling the two new STRICT nodes. I've corrected that.

I just realized I missed a couple of changes. Let me work on those and I'll update later.

Address review comment: Shrink the diff by eliminating unneeded use of else.

I've changed the patch so that calls to CreateFAdd() et al will give you constrained intrinsics if they are enabled. This required adding functions to enable/disable constrained-as-default plus calls to deal with the rounding mode and exception handling for contrained intrinsics.

In D53157#1304347, @uweigand wrote:

But given that there is still infrastructure missing in the IR optimizers, I also think that at least in the first implementation, we probably should go with the original approach and just use constrained intrinsics everywhere in the function, and possibly add some function attribute that prevent any cross-inlining of functions built with constrained intrinsics with functions built with regular floating-point operations.

In D53157#1303398, @cameron.mcinally wrote:

If we all agree upon that, then we simply have to treat the functions that modify the FPEnv, e.g. fesetexcept(...), as barriers. That way it does not matter if a FENV_ACCESS=OFF function is translated with constrained intrinsics or not, since nothing can be scheduled around these barriers.

In D53794#1302746, @uweigand wrote:

As I understand the approach, it can never introduce a new trap. Now of course, if the original source value is outside the range of the target integer type, some of the instructions introduced by this approach may trap, but in that case, it is expected for the fp_to_uint operation to trap somewhere.

If the original source value is in range however, then none of the instructions introduced here can ever trap:

	​    // Sel = Src < 0x8000000000000000
	​    // Val = select Sel, Src, Src - 0x8000000000000000
	​    // Ofs = select Sel, 0, 0x8000000000000000
	​    // Result = fp_to_sint(Val) ^ Ofs

The first is a compare that never traps. The select operations themselves also never trap. The first select in addition contains a subtraction, but if Src is in range for the fp_to_uint operation, that subtract cannot trap either. Finally, if Src is in range for fp_to_uint, then by construction the value Val will be in range for fp_to_sint, and therefore that operation cannot trap either.

In D53794#1278834, @uweigand wrote:

Now, the solution suggested in this patch is also interesting since it can be done without introducing new basic blocks (and therefore can be done in SelectionDAG, without needed a new pass). However, in order to generate efficient code we probably still need to get to the same assembler code we'd have gotten from that new pass -- but now instead the burden is on the back-end to recover the if-then-else block from a series of select statements. Not sure what is better here.

In D53157#1291978, @cameron.mcinally wrote:

In D53157#1291964, @kpn wrote:

I don't expect anyone would need to switch between constrained and regular floating point at an instruction level of granularity.

The Standard allows for this (to some degree). E.g. FENV_ACCESS can be toggled between nested compound statements.

In D53157#1290364, @andrew.w.kaylor wrote:

Rather than having separate methods to create the constrained versions of the intrinsics, why not have a way to set the constrained state in the IR builder and have the regular CreateFAdd et. al. functions use that to automatically create the constrained intrinsic versions? This would correspond to how fast math flags are handled.

In D53157#1290248, @cameron.mcinally wrote:

Adding the usual suspects: @andrew.w.kaylor @craig.topper @uweigand @hfinkel

I don't see the benefit of this change. The intrinsics are good enough to be functional right now (minus a few that are in progress), so I'm not sure we need IRBuilder functions. Am I missing something?

In D53157#1290044, @spatel wrote:

Should the builder's FastMathFlags setting apply to these calls? Is it worth adding an optional FMF parameter to these now, so we don't end up with duplicated API like we have for the regular FP binops?

Update to have tests in this patch.

Minor test changes.

In D43142#1285825, @andrew.w.kaylor wrote:

In D43142#1285789, @cameron.mcinally wrote:

In D43142#1285708, @andrew.w.kaylor wrote:

The only thing we need to worry about is making sure that FP operations don't migrate across calls or other FP operations that would break these semantics. Using the intrinsics after inlining takes care of that.

I don't agree with this. This is changing the semantics if we choose to inline a function by converting some operations into constrained intrinsics. In other words, an executable will have different behavior if we choose to inline or not. That's not ok.

Well, yes and no. In a region defined as FENV_ACCESS off the user has granted the compiler explicit permission to ignore FP status bits and exception behavior. Depending on how we optimize these regions the FP status bits may come out differently and exceptions may or may not be raised, but the user has promised not to unmask exceptions or look at status bits in that region. So if that part of the behavior of the code changes depending on whether or not we inline that's OK because that's what the user signed up for. In that sense the behavior hasn't changed any more than it does between -O0 and -O3.

In D43142#1285602, @cameron.mcinally wrote:

In D43142#1285565, @kpn wrote:

If all optimizations including constant folding, or at least optimizations on floating point, are delayed until after inlining then there's no problem.

I'll add that this is a ton of work. A binary Instruction can't currently have two Constant operands. So, ConstantFolding is baked into the Instruction implementation right now. If I'm mistaken, someone please correct me.

In D43142#1284977, @andrew.w.kaylor wrote:

In D43142#1284800, @hfinkel wrote:

In D43142#1284271, @cameron.mcinally wrote:

In D43142#1284190, @kpn wrote:

...

It's unclear to me how LTO (or other cross file inlining) would work here. I haven't given it much though until now. My knee-jerk reaction is that we shouldn't be inlining from a FENV_ACCESS=OFF to FENV_ACCESS=ON location.

I'm pretty sure that we decided that we couldn't (because once you inline the regular FP ops, there's no way to restrict their movement relative to the constrained intrinsics at the IR level).

I thought what we said was that if constrained FP intrinsics were used anywhere in a function then they had to be used everywhere in the function. So if you inline a function with FENV_ACCESS=ON into a function with FENV_ACCESS=OFF then the FP operations in the destination function need to be converted to constrained intrinsics, and if you inline a function with FENV_ACCESS=OFF into a function with FENV_ACCESS=ON then the FP operations in the function being inlined need to be converted. This conversion will have a detrimental effect on optimizations, so it would be nice to factor that into the inling cost, but at this point I'm mostly concerned about generating correct code.

In D43142#1285069, @cameron.mcinally wrote:

In D43142#1284977, @andrew.w.kaylor wrote:

I thought what we said was that if constrained FP intrinsics were used anywhere in a function then they had to be used everywhere in the function. So if you inline a function with FENV_ACCESS=ON into a function with FENV_ACCESS=OFF then the FP operations in the destination function need to be converted to constrained intrinsics, and if you inline a function with FENV_ACCESS=OFF into a function with FENV_ACCESS=ON then the FP operations in the function being inlined need to be converted. This conversion will have a detrimental effect on optimizations, so it would be nice to factor that into the inling cost, but at this point I'm mostly concerned about generating correct code.

I'm realizing that FENV_ACCESS is poorly designed.

Let's take this small example:

#include <fenv.h>
#include <stdio.h>

void bar();

#pragma STDC FENV_ACCESS ON
int main() {
  feenableexcept(FE_DIVBYZERO);
  bar();
  float x = 1.0/0.0;
  printf("main x=%lf\n", x);
}

#pragma STDC FENV_ACCESS OFF
void bar() {
  float x = 1.0/0.0;
  printf("bar x=%lf\n", x);
}

The FDiv in bar() would access the FP environment since main() enabled exceptions. But, since bar() is preceded with FENV_ACCESS=OFF, the Standard says that accessing the FP environment in bar() is undefined behavior. Since it's undefined behavior, the compiler can optimize as it wishes, including inlining bar() without modifying the FDiv.

In D43142#1284271, @cameron.mcinally wrote:

In D43142#1284190, @kpn wrote:

And couldn't we turn off constant folding in the IRBuilder when necessary?

I don't think that would work in all cases. E.g. a binary operator with two constant operands.

int main() {
  float z = 5.0/0.0;
  return z;
}

Well, not without a considerable effort to rework the IRBuilder at least. I'm fairly sure we can't produce an Instruction with two Constant operands right now.

Oh, of course FNeg will need to be emitted by the front end. And couldn't we turn off constant folding in the IRBuilder when necessary?

Isn't this pass needed since we don't have a way to prevent code movement between blocks using the pragma FENV_ACCESS ON and blocks that aren't? I suggest having it check for use of constrained intrinsics before making any changes.

Address review comments.

FWIW, I like it.

Personally, I'd like to see something where developer-only debug options show up in -help-hidden or similar for the correct executable: opt vs llc. Right now I'm pretty sure they don't. I don't have any opinion on how to make that happen, though.

In D53411#1269102, @uweigand wrote:

While you're at it, shouldn't we also add ROUND and TRUNC to complete the set?

Upload the correct diff this time. Sorry about the confusion!

Address review comments.

I forgot to address the template question. I've spend the past 18 years doing SystemZ backend work on multiple {,JIT} compilers. I'm not really in a position to answer your question about how to handle getting these FP bits down into C++ templates. I hope that won't hold up getting FENV_ACCESS working for C compilations.

The test case for this is in D52839, now.

Address review comments.

Update based on feedback to D52839: add missing AST (de)serialization support.

Ping

Correct obvious error with powi. Fix test and test both C and (partial) C++.

Split out changes as requested. This diff is just the four new intrinsics. The fptoui pass and the change to frem will be later.

In D43515#1229127, @craig.topper wrote:

Adding new constrained instrinsics and adding the pass should be separate patches I think. Changing the syntax of frem should be another patch.

Rebase. Ping.

In D51255#1214427, @jdoerfert wrote:

In D51255#1214424, @asl wrote:

I think we need to preserve the protocol. E.g. if the page is loaded via http we need to load css via http as well.

That sounds reasonable and is (afaik) implied by removing the hostname.
As mentioned before, I like the solution but I just don't know if that might cause problems somewhere else.

Is there a good reason to have the hostname in the URL? Why not just link to "/Users.html" instead of "https://llvm.org/Users.html", for example? Using the hostname is only needed when linking to a page on a different server.

In D50913#1212622, @cameron.mcinally wrote:

Did we decide that fast math flags can't be applied in the presence of constrained operations?

I haven't thought about this before today, but some of the flags we would want. E.g. reassoc/contract/afn.

In a pure trap-safe mode, contract/afn should be turned off. But, generally speaking, our users would prefer to see optimized code that misses some right-at-the-edge cases.

I just got done talking to one of our floating point math guys here.

In D50913#1205990, @cameron.mcinally wrote:

If the fsub that led to the FNEG node was not constrained in the first place then this seems like a valid transform.

Bah, sorry. My comment was ambiguous...

I meant that the FNEG->FSUB xform is "bad" in the context Ulrich proposed. For example:

The user writes a unary minus, -x, which is a bitwise operation and will never trap.

In D50913#1205893, @cameron.mcinally wrote:

I'm looking into the FNEG handling and found one bad xform in llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:

// Expand Y = FNEG(X) -> Y = SUB -0.0, X

That's not necessarily a show-stopper though. We could change this code to do a bitwise operation instead. That may even be faster than a FSUB, depending on the target of course.

In D50913#1205242, @uweigand wrote:

In D50913#1204810, @cameron.mcinally wrote:

I do see that you wrote -0-x. Is there something special about the -0?

I understand this is to map -0 to +0 and vice versa.

In D50913#1204826, @cameron.mcinally wrote:

User writes 0-x: Frontend maps to constrained FSUB
User writes -x: Frontend maps to FSUB

Yes, exactly.

Rebase. Ping.

This includes the change to pass the annotation token when FENV_ACCESS is turned off.

Replace accidentally lost comment.

Thanks for the fast turnaround! I don't have commit access, can I get you to commit it for me? And can I get you to put my email address <kevin.neal@sas.com> in the commit message?

Updated based on review.

Delete a line accidentally left in.

Missed one place in the documentation that needed updating.

This new diff adds changing the default lowering of STRICT_FP_TO_UINT to not allow speculative execution. I've also eliminated the rounding metadata from instructions when it wasn't needed.

At the request of my employer's legal department:
Copyright © 2018 SAS Institute Inc., Cary, NC, USA. All Rights Reserved.