[clang] Add support for option -ffp-eval-method and extend #pragma float_control similarly

[mibintc]

Summary

The Intel compiler supports the option "-fp-model=(source|double|extended)" which causes the compiler to use a wider type for intermediate floating point calculations. Also supported is a way to embed this effect in the source program with #pragma float_control(source|double|extended). This patch proposes to extend pragma float_control syntax, and also to support a new floating point option via "-ffp-eval-method=(source|double|extended)"

Note that the C floating point working group has proposed nearly identical pragma semantics in http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1974.pdf like this
#pragma STDC FENV_FLT_EVAL_METHOD width
I would like to add support for this also, but it's not in this patch. In the WG14 document, the standard widths are 0,1,2 representing source, double and extended precision. Negative values of width are reserved for the implementation.

The ICL option with description of semantics is at this url, https://software.intel.com/content/www/us/en/develop/documentation/cpp-compiler-developer-guide-and-reference/top/compiler-reference/compiler-options/compiler-option-details/floating-point-options/fp-model-fp.html
Quoting that here for easy reference "source
Rounds intermediate results to source-defined precision.
double
Rounds intermediate results to 53-bit (double) precision.
extended
Rounds intermediate results to 64-bit (extended) precision."

[mibintc]

clang/lib/Sema/SemaAttr.cpp

489

@rjmccall I would like to push a new value for FLT_EVAL_METHOD at the start of the scope of this pragma, then pop that value of the macro to restore the previous setting when the scope is exited, should I do that in ParsePragma? Can I do that by injecting "pragma push_macro(...)" into the token stream, do you have other suggestion or is there something similar in clang that I can study how to do this?

[mibintc]

@rjmccall Hoping you can take a look at this patch

[mibintc]

I found a way to get the macro FLT_EVAL_METHOD set properly, so this patch is ready for your review, thanks!

[aaron.ballman]

One question that came up in off-line discussions with @mibintc is whether the evaluation method should have an impact on constant folding or not. My intuition is that it should impact constant folding because it would be pretty strange for the evaluation at runtime to produce different results, but I don't have anything concrete to back my intuition up with.

clang/docs/UsersManual.rst

1530

clang/include/clang/Basic/LangOptions.h

244

FYI: it may be somewhat confusing that we have an enumerator with default in the name but that enumerator isn't the default.

clang/lib/Frontend/CompilerInvocation.cpp

4250

May want to update the comment above for why we're turning the option off here.

clang/lib/Lex/PPMacroExpansion.cpp

1716

It looks like some unrelated formatting changes snuck in.

[mibintc]

clang/include/clang/Basic/LangOptions.h

244

That's true. I had a heck of a time getting the initialization of the lang option setting correct: the setting should come from the TargetInfo if command line options don't override. That seems simple enough but there's somethng really odd about initialization. Adding the "default target" enumeral allowed me to get it initialized the way I wanted.

[mibintc]

clang/docs/LanguageExtensions.rst

3581

I'd like to add Intel ICL compatible syntax, but 'off' doesn't make too much sense to me. In the Intel compiler using'off' requires both ffp-exception-behavior to be ignore, and FENV_ACCESS to be off. If that's true then setting float_control(source|double|extended, off) is allowable and the semantics of float evaluation method set to a 4th level "float eval fast" which is loosely described in the commentary as "don't care". Ideally "float eval fast" would deliver the same results, in this context, as Microsoft's fp:fast command line option. I could make "off" an illegal setting, or warn that it's ignored. @andrew.w.kaylor do you have any input here?

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

3954

Can you please use the marshalling infrastructure for this? https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option (MarshallingInfoEnum)

[mibintc]

I rebased the patch and responded to review comments from @aaron.ballman and @jansvoboda11

[mibintc]

clang/include/clang/Basic/LangOptions.h

244

I redid the comments to eliminate the confusion

clang/lib/Sema/SemaAttr.cpp

489

Nevermind, I found a way to set the macro.

[aaron.ballman]

clang/docs/LanguageExtensions.rst

3586

clang/docs/UsersManual.rst

1528

clang/include/clang/Basic/LangOptions.h

236

clang/include/clang/Lex/Preprocessor.h

181

Otherwise, when LexExpandSpecialBuiltins is false, I think this winds up being an uninitialized pointer value.

clang/lib/Driver/ToolChains/Clang.cpp

2827

clang/lib/Lex/PPMacroExpansion.cpp

1743

Unrelated formatting change?

clang/lib/Sema/Sema.cpp

239

I don't think this cast is needed, is it? (It might make sense for the getFloatEvalMethod() return type to be int rather than unisnged though, given that we support negative values for implementation-defined semantics?)

243

Heh, so we have:

setCurrentFltEvalMethod
getFloatEvalMethod
getFPEvalMethod

I think we should pick a consistent spelling for float evaluation method and go with it. I don't know that work needs to happen as part of this patch (it can be a follow-up), but I think it'd help with readability.

clang/test/CodeGen/fp-floatcontrol-pragma.cpp

240

Can we also get some tests that show how this behaves with the less common floating-point types like float16, bfloat16, float128, half?

[mibintc]

Respond to @aaron.ballman 's review

[mibintc]

clang/include/clang/Lex/Preprocessor.h

181

thanks for catching this!

clang/lib/Lex/PPMacroExpansion.cpp

1716

clang-format really wants to fix this file

clang/lib/Sema/Sema.cpp

239

You are so right. i changed the type and modified the identifier like you suggested to make things more easily understandable

clang/test/CodeGen/fp-floatcontrol-pragma.cpp

240

I added tests fp16 and float128. I don't know if the other type names are supported in Intel, _Float16 isn't supported. I didn't see a typename half nor bfloat16 being used in any of the CodeGen tests, is this sufficient? if the predicate IsFloatingType is true and if the expression comes into UsualUnaryConversions then this new code should take effect

clang/test/Preprocessor/predefined-flteval-macro.c

1 ↗

(On Diff #343666)

Since FLT_EVAL_METHOD is no longer a typical predefined macro, I removed all the tests from clang/test/Preprocessor/init.c (see above) and created this to test all the combinations

[aaron.ballman]

I think the changes look good to me, but I'm not a floating-point expert either.

Adding @hubert.reinterpretcast for his floating-point expertise.

clang/test/CodeGen/fp-floatcontrol-pragma.cpp

240

I think this is fine (I picked the floating point names from Types.h when looking to see what other floating-point types exist) -- thanks for adding the new tests!

[mibintc]

I've rebased and applied clang-format. I'd like to push this, looking for your +1, thank you!

[aaron.ballman]

In D93769#2887857, @mibintc wrote:

I've rebased and applied clang-format. I'd like to push this, looking for your +1, thank you!

I've marked with my LGTM, but I'd still really appreciate a double-check from someone with more experience in floating-point before we land.

[MaskRay]

This breaks clang/test/Index/preamble-reparse-changed-module.m

http://45.33.8.238/linux/51638/step_7.txt

[MaskRay]

clang/test/Preprocessor/predefined-flteval-macro.c

332 ↗

(On Diff #360247)

So many clang invocations slow down testsuite execution.

Can some RUN lines be removed if their coverage value is low ?

[mibintc]

In D93769#2891545, @MaskRay wrote:

This breaks clang/test/Index/preamble-reparse-changed-module.m

http://45.33.8.238/linux/51638/step_7.txt

I saw this fail on console when i do "ninja check-clang" but when I run each step in this test separately there is no failure
the return code is 0
I don't understand what the failure is, how can I discover the problem? Thank you. I can revert the patch

[MaskRay]

In D93769#2891549, @mibintc wrote:

In D93769#2891545, @MaskRay wrote:

This breaks clang/test/Index/preamble-reparse-changed-module.m

http://45.33.8.238/linux/51638/step_7.txt

I saw this fail on console when i do "ninja check-clang" but when I run each step in this test separately there is no failure
the return code is 0
I don't understand what the failure is, how can I discover the problem? Thank you. I can revert the patch

Seems like a libclang crash (an assert failure) if you enable LLVM_ENABLE_ASSERTIONS=on.

You can reproduce with check-clang-index

[mibintc]

clang/test/Preprocessor/predefined-flteval-macro.c

332 ↗

(On Diff #360247)

OK. I was trying to demonstrate that my changes hadn't modified the macro settings across all

[mibintc]

In D93769#2891552, @MaskRay wrote:

In D93769#2891549, @mibintc wrote:

In D93769#2891545, @MaskRay wrote:

This breaks clang/test/Index/preamble-reparse-changed-module.m

Seems like a libclang crash (an assert failure) if you enable LLVM_ENABLE_ASSERTIONS=on.

You can reproduce with check-clang-index

Thanks for this, I'm building with assertions on now. This patch doesn't expand FLT_EVAL_METHOD in -E mode, I'm guessing that's why it fails. It can't expand the macro during -E because the context showing the value of the macro setting is only available in Sema. I haven't yet studied the test but do you know have an idea how I might be able to solve the problem?

[mibintc]

@MaskRay I experience that the failing test clang/test/Index/preamble-reparse-changed-module.m is non-deterministic and currently I cannot reproduce the fail. I added details in D95159. Are you able to reproduce if you clean your build area? Do you have a suggestion for how I can solve this problem? Thanks a lot

[zahiraam]

This comment was removed by zahiraam.

[matthewtff]

This CL breaks -E flow. I've created a repro: https://pastebin.com/fFuUdsfp
If you build it on linux with ToT clang like this: clang++ -target i386-unknown-linux-gnu repro.cc -o repro.bin
and then run the binary you'll get output "Hello, One". But if you make a roundtrip via preprocessing:
clang++ -target i386-unknown-linux-gnu -E repro.cc > repro_pp.cc
clang++ -target i386-unknown-linux-gnu repro_pp.cc -o repro_pp.bin
And then run ./repro_pp.bin, you'll get output "Hello, Three". So now the binary differs.

If you revert this CL, then both ways of compiling would give the same binary, that would output "Hello, One".

[zahiraam]

In D93769#2970097, @matthewtff wrote:

This CL breaks -E flow. I've created a repro: https://pastebin.com/fFuUdsfp
If you build it on linux with ToT clang like this: clang++ -target i386-unknown-linux-gnu repro.cc -o repro.bin
and then run the binary you'll get output "Hello, One". But if you make a roundtrip via preprocessing:
clang++ -target i386-unknown-linux-gnu -E repro.cc > repro_pp.cc
clang++ -target i386-unknown-linux-gnu repro_pp.cc -o repro_pp.bin
And then run ./repro_pp.bin, you'll get output "Hello, Three". So now the binary differs.

If you revert this CL, then both ways of compiling would give the same binary, that would output "Hello, One".

I do see a previous comment from Melanie mentioning this issue:
"Thanks for this, I'm building with assertions on now. This patch doesn't expand FLT_EVAL_METHOD in -E mode, I'm guessing that's why it fails. It can't expand the macro during -E because the context showing the value of the macro setting is only available in Sema. I haven't yet studied the test but do you know have an idea how I might be able to solve the problem?"
It looks like Melanie asked the questions but it wasn't resolved.

[matthewtff]

It looks like Melanie asked the questions but it wasn't resolved.

Can this CL be reverted and relanded later with a fix? This breakage is crucial for a lot of remote/distributed compilation systems.

[erichkeane]

In D93769#2972151, @matthewtff wrote:

It looks like Melanie asked the questions but it wasn't resolved.

Can this CL be reverted and relanded later with a fix? This breakage is crucial for a lot of remote/distributed compilation systems.

Did someone perhaps fix this in trunk? I don't see it on godbolt: https://godbolt.org/z/r76rx5Evd

See the preprocessed version ends up being:

int main() {

std::cout << "Hello, " "One" << std::endl;

}

[aaron.ballman]

In D93769#2972151, @matthewtff wrote:

It looks like Melanie asked the questions but it wasn't resolved.

Can this CL be reverted and relanded later with a fix? This breakage is crucial for a lot of remote/distributed compilation systems.

I think that's reasonable, as Melanie recently retired from Intel and so it may take a bit to get the correct fix. Someone should double-check that this patch did *not* make it to the Clang 13 branch, too. By my understanding the branch was at 1:45am Jul 28 and this patch landed at 10:51am Jul 28 and so we should be good, but best to double-check.

[erichkeane]

In D93769#2972203, @erichkeane wrote:

In D93769#2972151, @matthewtff wrote:

It looks like Melanie asked the questions but it wasn't resolved.

Can this CL be reverted and relanded later with a fix? This breakage is crucial for a lot of remote/distributed compilation systems.

Did someone perhaps fix this in trunk? I don't see it on godbolt: https://godbolt.org/z/r76rx5Evd

See the preprocessed version ends up being:

int main() {

std::cout << "Hello, " "One" << std::endl;

}

Ah, nvm, I see what I did wrong. The test requires 32 bit to fail, since there is an #elif branch of the #ifdef __FLT_EVAL_METHOD__. Disregard.

[matthewtff]

Did someone perhaps fix this in trunk? I don't see it on godbolt: https://godbolt.org/z/r76rx5Evd

You've missed the -target argument. It still reproduces with correct arguments: https://godbolt.org/z/1ozhjYvhf