I submitted the test improvements separately, i'm going to push this version, thanks all

you're right about the regex syntax. i think the 4088 is over the max repetition count and there's also a problem with balanced. not sure what i did wrong here. if you have a chance to look at this otherwise i'll toil over the weekend, i'm off for the day--cheers. here's what i'm trying now just to see if it would work. can i nest repetition counts?

Fixed test case according to @dblaikie 's suggestion and input, thanks a lot David, the test case is certainly a lot easier to read and understand now. Anything else?

I'm sorry, I don't see how to build up an identifier with an arbitrary number of characters using the token pasting, for example an ident with 4095 characters is not a power of 2. I tried pasting together X2048 X1024 X512 etc but that doesn't work
fu.cpp:12:18: error: pasting ")" and "X16" does not give a valid preprocessing token
#define FUN X32(x) ## X16(x)

^

fu.cpp:13:1: note: in expansion of macro ‘FUN’
I want the test case to show the transition between when md5 is not needed and when it's needed, i want to build strings that might not be power of root string. I could add more comments to the test case, perhaps, "the strlen of the Microsoft mangled name is X" for the one case, and "the strlen of ... is X+1" for the other.

In D90714#2374913, @dblaikie wrote:

Since the same code is used to mangle all these things, probably just test one of them?

Could use macros to stamp out longer names without having to write them out manually?

In D90714#2372458, @dblaikie wrote:

Test case?

Add lit test case, based on test case in bug report

respond to @aaron.ballman 's review

The failing unit test reported above, sizes.cpp, looks like it should be an Expected Fail

In D87528#2357970, @pengfei wrote:

I agreed with Craig. Emitting constrained intrinsics on an unsupported target may result in problems. It's better to check if it is supported and prevent from the front end.

In D90316#2359366, @kpn wrote:

I also added "-fexperimental-strict-floating-point" which makes it possible to write tests for a target when bringing up the support on that target. I think it would be confusing for that flag to work on command line arguments but not pragmas.

Modified according to @kpn suggestion: if the command line has option fexperimental-strict-floating-point, the float pragmas will not be ignored.

Actually kludging it by just removing the assert isn't going to work. I'll ping Pengfei to see about developing a patch for this problem.

bug report

Thank you. I don't know the solution but I can remove the assert and create a bug report to have it resolved.

I updated the test cases, added documentation about constant folding to the UserManual. I'll push the patch tomorrow if I don't hear to the contrary thanks everybody!

I made the change requested by @rsmith to HandleIntToFloatCast, avoiding the check if Info.InConstantContext, then I corrected 2 tests so that -verify matches.

clang/test/CXX/expr/expr.const/p2-0x.cpp
clang/test/Parser/pragma-fenv_access.c

In D87528#2270541, @rsmith wrote:

@rsmith asked "I don't see changes to the constant evaluator". The semantic rules for enabling this pragma require that strict or precise semantics be in effect., see SemaAttr.cpp And the floating point constant evaluation doesn't occur when strict or precise

What do you mean "the floating point constant evaluation doesn't occur when strict or precise"? I don't see any code to do that in ExprConstant.cpp, neither in trunk nor in this patch. The constant evaluator certainly is still invoked when we're in strict or precise mode. I would expect the evaluator would need to check for FP strictness whenever it encounters a floating-point operator (eg, here: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L13340), and treat such cases as non-constant.

1.0/3.0 is now static folded. @rsmith Did you change your mind about this? Is static folding what you want here?

I'd like to see a C++ test for something like this:

float myAdd() {
#pragma STDC FENV_ACCESS ON
    static double v = 1.0 / 3.0;
    return v;
}

If that generates a runtime guarded initialization for v, then I'd be a bit more convinced we're getting this right.

I added StrictFPAttr clang function attribute accroding to @rjmccall 's suggestion instead of using a boolean on the function decl, rebased, and responded to other code review comments. will add replies inline

In D88498 @rsmith wrote,
s far from clear to me that this is correct in C++. In principle, for a dynamic initializer, the rounding mode could have been set by an earlier initializer.

I rebased the patch
I added documentation to UserManual showing that -ffp-model=strict enables FENV_ACCESS
I removed ActOnAfterCompoundStatementLeadingPragmas since it was redundant with work being done in ActOnCompoundStmt

ping!

I added a sentence in the clang UserManual pointing out that ffp-model=strict automatically enables FENV_ACCESS. I changed checkFloatingPointResult the way @sepavloff suggested to solve the LIT failure in clang/test/AST/const-fpfeatures.cpp, as long as the ExceptionBehavior is Ignore, the FENV_ACCESS is Off and RoundingMode isn't Dynamic, it's OK to fold even if the Status isn't OK. Now check-clang is running clean.

For the LIT test clang/test/AST/const-fpfeatures.cpp, currently failing in this patch, the variables V1 and others are initialized via call to "global var init" which performs the rounding at execution time, I think that's correct not certain.

In the previous version of this patch, I enabled FENV_ACCESS if frounding-math, but @sepavloff commented that this deduction is not correct. I changed the logic to check for the "ffp-model=strict" settings, and if those settings are in effect from the command line, then I also enable fenv_access. To repeat what I've said elsewhere, this is the documented behavior of the "/fp:strict" Microsoft command line option.

I neglected to hit the "submit" button last week, these are the inline questions I have.

I patched my workspace with D88498 from Sept 29. I fixed some nits and also enabled FENV_ACCESS when the command line option specifies frounding-math. This corresponds to Microsoft's documentation of their option /fp:strict https://docs.microsoft.com/en-us/cpp/preprocessor/fenv-access?view=vs-2019 "The [/fp:strict] command-line option automatically enables fenv_access." Also if #pragma to enable fenv_access is seen, RoundingMode is set to Dynamic
I have some questions I'll add them as inline comments

refer to https://reviews.llvm.org/D88498

thanks for this!

In D87528#2297647, @sepavloff wrote:

In D87528#2295015, @mibintc wrote:

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

In D87528#2297647, @sepavloff wrote:

In D87528#2295015, @mibintc wrote:

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

In D87528#2270502, @sepavloff wrote:

@sepavloff Is it OK if I continue work on this item? Not sure about the protocol when continuing someone else's patch.

It is OK for me. There is also an action in Phabricator "Commandeer Revision" to transfer ownership on a revision item.

I don't think however that the implementation in frontend is the main obstacle for enabling the pragma. It is the part of the standard and is user visible, so clang must provide satisfactory support so that users could try this feature in real applications. This support mainly depends on the support of constrained intrinsics in IR and codegen.

One of the probable ways to confirm the support is to build some pretty large project that uses floating point operations extensively, build it with option -fp-model=strict and check if it works. A good choice could be SPEC benchmarks. It would provide us with not only evidence of support but also with number how strict operations slow down execution. Maybe other projects may be used for this purpose, but I don't know such.

Note, @sepavloff is working on overlapping concerns here, D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

Responded to @rsmith's review

not needed

LGTM - thanks

I pulled out the isStrictFP boolean from EvalInfo and used the FPOptions when visiting floating point BinaryOperator, CastExpr and builtin CallExpr. I created the diagnostic note explaining why constant evaluation failed. Not certain about the language rules C vs C++.

This update uses context information from Expr->getFPFeaturesInEffect() to disable fp constant folding in ExprConstant.cpp

In D69272#2271338, @sepavloff wrote:

In D69272#2268337, @mibintc wrote:

I am working on a task to ensure that clang is doing floating point constant folding correctly.

Could you please share your plans on it? I recently also started implementing constant folding in ExprConstant.cpp. I have not made anything substantial yet, so I can easily switch to another task. Do you have any estimation when you could prepare the first version of the patch?

I've been given a vague assignment, something along the lines "investigate floating point constant folding and make sure that the semantics are correct. " In the Intel ICL compiler, there were some circumstances of the semantics not being correct. I saw Richard's comments in this review, and Intel also needs FENV_ACCESS implemented so I thought I'd start here. I'm not a floating point expert, but of course some of my colleagues at Intel are! I am pretty slow but it's my area of focus.

But sometimes, if the floating point semantics are set to 'strict', even tho' folding has occurred successfully in ExprConstant.cpp, when i look at emit-llvm, there is arithmetic emitted for the floating point expression;

I used a bit different approach, may be it could be useful for you too. An initializer for global variable must be a constant, so things like const xxx = 1.0 + 2.0 are evaluated. No llvm arithmetic occurs in the resulting ll file. Using pragma STDC FENV_ROUND floating point environment may be set to non-default state, which constant evaluator must use.

When I implemented clang #pragma float_control, I noticed that initialization expressions in classes were not subject to the pragma's that are active in the source file. Those expressions are pulled out and processed differently than the function bodies. I'll upload later today a patch that uses Expr->getFPFeaturesInEffect() to inhibit constant folding in ExprConstant.cpp.

In D69272#2268387, @kpn wrote:

Say, in D80952 I added support for disabling strictfp support when a target doesn't support it. But it only applies to command line arguments.

Is there any chance at all that relevant pragmas can also be disabled with the warning in the same cases?

This is definitely a good idea.

I'll look into it, thank you

Hello, I rebased this and made a few changes here, https://reviews.llvm.org/D87528 ; I added a question about floating point constant folding in that review, I'm going to duplicate it here,

In D87049#2253150, @dblaikie wrote:

I think it'd be helpful to discuss what the DWARF does and should look like in this case (I wonder if, maybe, the better solution is in the DWARF consumer - to do name lookup out through the lambda) - could you provide some summarized llvm-dwarfdump output comparisons between Clang (with/without the patch applied) and GCC's output?

with updates from @yaxunl , i'm planning to push this

In D81869#2116752, @yaxunl wrote:

Would you please add the following lit test

diff.pch.txt1 KBDownload

If you change FastMath, FiniteMathOnly and UnsafeFPMath to COMPATIBLE_LANGOPT, the test should pass.

I decided that I shouldn't make float options that define a macro, like -ffast-math, as BENIGN_LANGOPT, I made ffp-contract= , fp-exception-behavior and rounding-mode BENIGN, I modified the pch test case to test that the benign command line floating options on the pch-create don't affect compilations that use the pch file.

I need to make another revision that makes a couple more of the fp options, like ffp-contract, "benign" and add a test case that demonstrates fp options don't carry over from pch-create to pch-use

The difference between this patch and the earlier one today is that I modified the new test case

I responded to review from @riccibruno and @rjmccall : I put the dump() routines into the .cpp file and changed them to use the x-macros. I moved CXXOperatorCall.FPOptionsOverride from the Expr bits into the OperatorCall itself. I added the operator call test case suggested by John. I must be mistaken about the bug, the operator call addition does account for the FPFeatures in both the unmodified compiler and the patched compiler.

@rjmccall I added some inline questions and comments for you. Thanks a lot for your review.

This revision rewrites FPOptionsOverride like @rjmccall suggests. There are a couple problems, i'll add inline comments in the trouble areas

This version passes all the lit tests, i believe it's functional tho' maybe not elegant.
I still need to add a test case that PCH behaves as desired, and that the floating point command line options from PCH create do not clobber command line options from compilations that use a pch

@rjmccall You suggested that the FPFeatures could be delta instead of absolute settings, I think this is approximately what you mean. This is a rough patch employing that idea, would welcome your early comments. There are 2 clang tests that are failing with this patch, I need to dig into those.

I documented the issue reported by @yaxunl here, https://bugs.llvm.org/show_bug.cgi?id=46166, and take ownership of the bug. Thanks for the report.

Thanks for cleaning this up!