Excellent! Thanks Matt!

Looks like the test case didn't make it over.

I'm happy to see the uses of ReplaceAllUsesOfValueWith() gone. Having nodes be only halfway spliced into the tree for a while seemed like it could cause bugs later. I imagine the scalar legalizer might could benefit from similar changes at some point in the future.

My pleasure!

Update now-failing tests.

In D69272#1794630, @sepavloff wrote:

Putting restriction on use of the pragma is of course, a temporary solution, it is not usable in all cases. But for some cases it is usable in production code. Where small pieces of code may be extracted into separate functions, this solution can provide tolerable performance loss, if most part of the program doesn't use constrained intrinsics. Warning prevents users from false expectations. Use in production code ensures further development of the feature.

Full-fledged solution requires full support of constrained intrinsics in optimizations. It is not clear how large this work is, but odd are that it would require substantial efforts. It this case usable implementation of 'pragma STDC FE_ACCESS' would postponed. The restricted solution can be implemented much faster and it does not impede development of the full-fledged one.

Imagine a case where some function "X()" under the #pragma and a non-default FP environment calls a function "Y()" in a different TU and not under the #pragma. Then the programmer moves "Y()" into a header file. Under your proposal Y() would get different FP environments at run time simply because it was moved from a different TU into a header file. Surprise!

In D69798#1794639, @sepavloff wrote:

In D69798#1793237, @kpn wrote:

In D71742#1794638, @sepavloff wrote:

In D71742#1793243, @kpn wrote:

I don't see the need. Changing the FP environment in a mixed environment program is the responsibility of the programmer, and standard calls already exist for this.

This is about inlining. In the code like this:

double f1(double x, double y) {
  return x + y;
}
double f2(double x, double y) {
  #pragma STDC FENV_ACCESS ON
  ...
  return f1(x, y);
}

compiler might inline call to f1 in f2. However the inlined function f1 expects default FP environment but is called in some other one.

I don't see the need. Changing the FP environment in a mixed environment program is the responsibility of the programmer, and standard calls already exist for this.

In D69798#1792906, @sepavloff wrote:

In D69798#1792237, @craig.topper wrote:

In D69798#1792231, @sepavloff wrote:

When function that uses default FP environment is inlined into strictfp function, code of the former will be executed in the FP environment set in the strictfp function. To fix this behavior the FP environment should be saved upon entry to the inlined function, FP environment reset to default state, and the saved state must be restored upon leaving the inlined function.

So if the function is inlined it would use the reset state, but if it doesn't get inlined it would use the caller's state. Doesn't that mean whether or not the compiler inlines the function changes the behavior of the program?

It is so now. If a code in which #pargma STDC FENV_ACCESS ON acts calls an external function, it would be executed in caller's FP environment. This is wrong if the callee expects default one. We could put get_fenv, reset_fenv and set_fenv, introduced in D71742, around all calls, unless the called function has strictfp attribute, or we know that it does not use FP operation. It however could create unneeded code, which is bad for performance. We need to elaborate proper solution.

In D69272#1792856, @mibintc wrote:

In D69272#1717021, @kpn wrote:

Is there a way forward to support having the #pragma at the start of any block inside a function? The effect won't be restricted to that block, true, but the standard does say the #pragma is allowed.

Please clarify: I understand that the backend wants all floating point operations to be built using Floating Point Constrained Intrinsics if any operations use constrained intrinsic. But I thought that, if the constraint were only to apply to a block within the function, that the operations outside the block would be written with the default setting for rounding mode and exception behavior, and the operations inside the constrained block would be created with different settings. Like this

LGTM. Thanks for the fix!

Address review comments.

Address review comments. I've eliminated the need to call SelectionDAG::UnrollVectorOp() and therefore eliminated the changes to that function.

Address review comments, with changes to LegalizeDAG.cpp and LegalizeVectorOps.cpp.

Huh. I forgot to submit my comments earlier.

Rebase. Add missing tests.

Eliminate write-only variable.

Update for review comments.

Update for review comments.

Prep to update patch.

I think this is a good idea.

Redundant now.

@mibintc: This ticket is redundant now, correct? This was solved in the re-push of the command line option changes?

The calls DAG.UnrollVectorOp() can't be used in place of DAGTypeLegalizer::ReplaceValueWith() because then the replaced node doesn't get properly deleted. This then results in an assertion failure later because the replaced node wasn't legal. And having SelectionDAG call into DAGTypeLegalizer doesn't seem like a winning idea, at least not without some plumbing. So today I'm going to punt on merging those functions together and just post what I've got. After breakfast sometime.

I'm still working on this ticket daily! I'm trying to merge the two vector unrolling functions like Ulrich suggested. But I ran into problems that lead me to think we may have a serious issue lurking that we'll need to fix. That's what I've been working on: trying to understand the issue and see if it needs further investigation.

Update for review comments. Use MERGE_VALUES node. Eliminate hacks to avoid x86 custom lowering. This means more x86 target support.

In D70451#1752177, @mibintc wrote:

It works for me too. I need to send an update for my new test case fpconstrained.cpp

In D70451#1752027, @rjmccall wrote:

This seems like a much more reasonable approach in general. Frontend authors who intend to use constrained FP need to be making broader changes to how they emit IR in order to satisfy the requirements of the intrinsics.

Have we been able to extract a test case which causes the insertion into an unparented basic block? If that's happening in Clang IRGen, I consider that a bug. I can't speak for LLVM passes, though; they do silly things for obscure reasons.

In D62731#1750427, @mibintc wrote:

In D62731#1750412, @kpn wrote:

Does anyone think a warning is appropriate because the new flags are exercising experimental, incomplete code in both clang and llvm? The warning would be removed when we believe the feature is complete and ready to use.

@kpn Can you say more about "incomplete code in ... clang". I don't know what's missing from clang.

D62731 is required for testing this patch.

Does anyone think a warning is appropriate because the new flags are exercising experimental, incomplete code in both clang and llvm? The warning would be removed when we believe the feature is complete and ready to use.

In D70096#1744035, @kpn wrote:

Does strictfp need to be on function declarations? Currently it isn't.

Drop the verbiage about exceptions. Update ticket description and title.

Does strictfp need to be on function declarations? Currently it isn't.

In D70096#1741280, @andrew.w.kaylor wrote:

In D70096#1741213, @kpn wrote:

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

How would we do that?

I like the switch to using update_llc_test_checks.py. Sure, I'll rebase, run that script and it'll be in the next round of this patch when I can.

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

Address review comments. Add missing IR verifier pieces. Add the remaining test cases.

It looks like D69798 may be more complete patch. Let me take this off everyone's queue while that one is in flight.

The IRBuilder handles mapping non-constrained operations to constrained operations already. So a front-end just needs to enable strict mode in the IRBuilder and most everything "just works". And clang uses the IRBuilder, so we're good there. Are you working on a front-end that doesn't use the IRBuilder?

It looks like I am going to have to do some Custom lowering work after all. Plus the changes to the IR verifier are missing.

Address review comment: rework comment to stand out.

Address review comments.

In D69275#1724146, @cameron.mcinally wrote:

This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.

Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.

Added missing documentation. Added a case I missed earlier.

Lacks documentation. Sorry. I'm working on that now.

In D69312#1718371, @rjmccall wrote:

What does the call attribute mean?