Page MenuHomePhabricator

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

Authored by mibintc on Dec 23 2020, 9:50 AM.

Details

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."

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mibintc requested review of this revision.Dec 23 2020, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 9:50 AM
mibintc edited the summary of this revision. (Show Details)Dec 23 2020, 9:51 AM
mibintc edited the summary of this revision. (Show Details)
mibintc added inline comments.Dec 23 2020, 10:00 AM
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?

@rjmccall Hoping you can take a look at this patch

mibintc updated this revision to Diff 321756.Feb 5 2021, 7:14 AM
mibintc retitled this revision from RFC [clang] Add support for option -ffp-eval-method and extend #pragma float_control similarly to [clang] Add support for option -ffp-eval-method and extend #pragma float_control similarly.
mibintc edited the summary of this revision. (Show Details)
mibintc added a reviewer: aaron.ballman.

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

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 added inline comments.Feb 8 2021, 7:25 AM
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 added inline comments.Feb 8 2021, 8:43 AM
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 added inline comments.Mar 9 2021, 7:27 AM
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 updated this revision to Diff 342792.May 4 2021, 10:17 AM

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

mibintc marked 5 inline comments as done.May 4 2021, 10:23 AM
mibintc added inline comments.
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 added inline comments.May 6 2021, 6:53 AM
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 updated this revision to Diff 343666.May 7 2021, 6:40 AM

Respond to @aaron.ballman 's review

mibintc marked 9 inline comments as done.May 7 2021, 6:52 AM
mibintc added inline comments.
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

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!

Matt added a subscriber: Matt.May 19 2021, 11:15 AM
mibintc updated this revision to Diff 359847.Jul 19 2021, 11:15 AM
mibintc marked 3 inline comments as done.
mibintc added a reviewer: zahiraam.

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

aaron.ballman accepted this revision.Jul 20 2021, 8:16 AM

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.

This revision is now accepted and ready to land.Jul 20 2021, 8:16 AM
This revision was landed with ongoing or failed builds.Jul 20 2021, 1:02 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJul 20 2021, 1:34 PM

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

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

MaskRay added inline comments.Jul 20 2021, 1:35 PM
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 ?

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

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 added inline comments.Jul 20 2021, 1:39 PM
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 reopened this revision.Jul 20 2021, 1:43 PM
This revision is now accepted and ready to land.Jul 20 2021, 1:43 PM

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?

@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

This comment was removed by zahiraam.
matthewtff added a subscriber: matthewtff.EditedAug 27 2021, 2:55 PM

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".

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.

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.

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;

}

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.

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.

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