Page MenuHomePhabricator

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

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

mibintc created this revision.Dec 23 2020, 9:50 AM
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
487

@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
1488
clang/include/clang/Basic/LangOptions.h
234

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
4203

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
234

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
3394

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
3929

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
234

I redid the comments to eliminate the confusion

clang/lib/Sema/SemaAttr.cpp
487

Nevermind, I found a way to set the macro.

aaron.ballman added inline comments.May 6 2021, 6:53 AM
clang/docs/LanguageExtensions.rst
3399
clang/docs/UsersManual.rst
1486
clang/include/clang/Basic/LangOptions.h
226
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
2803
clang/lib/Lex/PPMacroExpansion.cpp
1744

Unrelated formatting change?

clang/lib/Sema/Sema.cpp
216

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?)

220

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
216

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

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.Wed, May 19, 11:15 AM