Page MenuHomePhabricator

[RFC] Add support for pragma float_control, to control precision and exception behavior at the source level
Needs ReviewPublic

Authored by mibintc on Jan 16 2020, 6:38 AM.

Details

Summary

Intel would like to support #pragma float_control which allows control over precision and exception behavior at the source level. This pragma is supported by both the Microsoft compiler and the Intel compiler, and our customers have found it useful. This message is to describe the pragma, provide the patch for review, and request feedback from the community.

As the pragma was originally defined in the Microsoft compiler, the pragma supports a stack of settings, so that you can use push and pop to save and restore the state. That functionality is already in clang and this patch piggy-backs on that support (reference PragmaStack and PragmaMsStackAction). The Microsoft compiler supports it only at file scope, but the Intel compiler, and this patch, supports the pragma at either file scope or at the beginning of a compound statement. Clang currently provides support for pragma fp_contract which enables floating point contraction--also at file scope and compound statement, and uses FPFeatures on certain expression nodes to hold the pragma settings. This patch piggy-backs FPFeatures, adding fields to show the setting of "float_control(precise)" and "float_control(except)"

This patch includes an update to the pragma documentation, to summarize, using pragma float_control(precise) is like enabling ffp-model=precise for a region of the program. pragma float_control(except) is like enabling ffp-exception-behavior=strict/ignore for a region of the program.

Diff Detail

Event Timeline

mibintc created this revision.Jan 16 2020, 6:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 16 2020, 6:38 AM
sepavloff added inline comments.Jan 16 2020, 10:38 PM
clang/docs/LanguageExtensions.rst
3048

Floating-point precision refers to the number of bits in mantissa, here the term precise floating-point semantics looks more appropriate.

3073

Blank line is needed after the end of paragraph.

clang/include/clang/AST/Decl.h
180 ↗(On Diff #238473)

What is the purpose of this class? Do you plan to extend it? If you need to restrict the scope of constants like FC_Unknown it is better to use scoped enumerations.

clang/include/clang/Basic/DiagnosticParseKinds.td
1306

We already have several pragmas that require the same restriction (clang fp, STDC FENV_ACCESS) and will add some more (STDC FENV_ROUND), probably it makes sense to use generic message and supply pragma name as argument?

clang/include/clang/Sema/Sema.h
555

Why typedef, not simply struct FpPragmaStateType?
Usually we use C++ style of struct declarations.

clang/lib/CodeGen/CodeGenFunction.cpp
120

Is clang:: necessary here? The file already has using namespace clang;.

135

Ditto.

clang/lib/Parse/ParsePragma.cpp
2546

Does such treatment allow a pragma like:

#pragma #pragma float_control(except, on), push

The comment to PragmaFloatControlHandler::HandlePragma says it is valid.

clang/lib/Sema/SemaAttr.cpp
430

push cannot be combined with precise?

clang/lib/Sema/SemaExpr.cpp
13133

The standard says that static initializers execute in default FP mode.

mibintc marked 3 inline comments as done.Jan 17 2020, 8:20 AM

@sepavloff Thanks a lot for your comments. I added a few replies and I have one question, added inline.

clang/lib/Parse/ParsePragma.cpp
2546

Yes, #pragma float_control(except, on, push) is allowed. That's inherited from the Microsoft pragma of the same name. I need to change the .rst documentation about this. #pragma float_control(push) or #pragma float_control(pop) is also supported. Here's a link to the Microsoft doc, https://docs.microsoft.com/en-us/cpp/preprocessor/float-control?view=vs-2019

clang/lib/Sema/SemaAttr.cpp
430

Yes it can be combined: #pragma float_control(precise, push); the "push" is coded into the Action, the action can be either "set" or "push". I'm using pre-existing code in clang which provides support for microsoft-style pragma push/pop/set. For example, clang supports the Microsoft pragma code_seg which supports a stack of code segment names.

clang/lib/Sema/SemaExpr.cpp
13133

The standard says ...

Are you sure about this one? Can you please provide the standards reference so I can study it?

sepavloff added inline comments.Jan 17 2020, 9:10 AM
clang/lib/Sema/SemaExpr.cpp
13133

The standard says ...

Are you sure about this one? Can you please provide the standards reference so I can study it?

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf, F.8.5:

... All computation for initialization of objects that have static or thread storage duration is done (as if) at translation time.

F.8.2:

During translation the IEC 60559 default modes are in effect:
— The rounding direction mode is rounding to nearest.
— The rounding precision mode (if supported) is set so that results are not shortened.
— Trapping or stopping (if supported) is disabled on all floating-point exceptions.
mibintc marked 2 inline comments as done.Mon, Jan 20, 9:01 AM

Added inline reply

clang/lib/CodeGen/CodeGenFunction.cpp
120

Yes the clang:: is necessary here, without it there are undefined symbols at link time. the using modifies name lookup but not declaration.

sepavloff added inline comments.Mon, Jan 20, 10:40 PM
clang/lib/Parse/ParsePragma.cpp
2561

Probably using Actions.getCurScope() can help to recognize file scope.

mibintc marked an inline comment as done.Tue, Jan 21, 11:47 AM
mibintc added inline comments.
clang/lib/Parse/ParsePragma.cpp
2561

Thanks for the suggestion, I (Actions.getCurScope()==0) to test for file scope, but that didn't work either. I put a workaround into the test case CodeGen/fp-floatcontrol-pragma.cpp, the forward class declaration ResetTUScope. If the reset is there, then the pragma is recognized to be at file scope.

mibintc updated this revision to Diff 239409.Tue, Jan 21, 12:41 PM

Respond to review from @sepavloff

mibintc marked 5 inline comments as done.Tue, Jan 21, 12:45 PM
mibintc added inline comments.
clang/lib/Sema/SemaExpr.cpp
13133

Thanks for the pointer to the reference. The desired semantics of the pragma may differ from the standard. For example I tried this test case with the fp_contract pragma, and the pragma does modify the semantics of the floating point expressions in the initializer. So this issue is still a problem in this patch.

// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s

#pragma STDC FP_CONTRACT ON
float y();
float d();
class ON {
float z = y() * 1 + d();
CHECK-LABEL: define {{.*}} void @_ZN2ONC2Ev{{.*}}
CHECK: llvm.fmuladd.f32{{.*}}
};
ON on;

#pragma STDC FP_CONTRACT OFF
class OFF {
float w = y() * 1 + d();
CHECK-LABEL: define {{.*}} void @_ZN3OFFC2Ev{{.*}}
CHECK: fmul float
};
OFF off;

I don't see tests for correctness of the pragma stack (pragma float_control(... push), pragma float_control(pop)). Can you add them?

clang/lib/Parse/ParsePragma.cpp
2561

Scope always exists, so the correct way to check if it refers to translation unit is something like: Actions.getCurScope()->getParent() == nullptr.

clang/lib/Sema/SemaExpr.cpp
13133

This is an interesting example. However there is no contradiction with the standard. The standard speaks about floating point environment, which on most processors are represented by bits of some register(s). The pragma STDC FP_CONTRACT does not refer to the FPEnv, but is an instruction to the compiler how to generate code, so it affects code generation even in global var initializers.

What TBD here means? Do you think this code may be somehow improved?

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

You need to extract the tests that check error generation from this file and put them into clang/test/Parser.

mibintc marked 2 inline comments as done.Wed, Jan 22, 1:07 PM

A couple inline replies to @sepavloff ; I'll be uploading another revision.

clang/lib/Parse/ParsePragma.cpp
2561

I tried this also: (Actions.getCurScope()->getParent() != nullptr) but that also failed to detect current scope is file scope. In the debugger, where the pragma occurs immediately after a function definition, I can see that CurScope is still the function body. The transition to outer scope must not yet have occurred. I can investigate.

clang/lib/Sema/SemaExpr.cpp
13133

I wasn't yet certain about the interpretation of the pragma on the initializatin expressions. Today I did some testing with ICL and CL and it seems the pragma has no effect on the initialization expressions that occur within constructors in classes at file scope. So I'll remove the TBD and the current behavior in this patch wrt this question is OK.

mibintc updated this revision to Diff 240245.Fri, Jan 24, 10:42 AM
mibintc marked 3 inline comments as done.Fri, Jan 24, 10:57 AM

I don't see tests for correctness of the pragma stack (pragma float_control(... push), pragma float_control(pop)). Can you add them?

I added a test case for the stack of float_control, and it's also hitting the problem with the function scope not being closed by the right brace. This patch is useless until I tackle that problem. In the test case I put in bogus declarations which force the function scope to close. There are some pragmas called "range pragmas" that affect the functions in a source location range in the file. I don't know if I could mix that idea with the MS pragma stack that I'm using to push/pop the pragma settings, I'm doubtful.

clang/lib/Sema/SemaExpr.cpp
13133

I removed the TBD, thanks.

Could you please rebase the patch against current master?
The commit "4aea70ed3292: [FPEnv] Extended FPOptions with new attributes" changed layout of FPOptions, which affects your patch.

mibintc updated this revision to Diff 240638.Mon, Jan 27, 10:32 AM

rebase per @sepavloff request

It's not clear to me from reading this how the "precise" control is going to work with relation to the fast math flags. I don't think MSVC allows the granularity of control over these flags that we have in clang, so maybe they don't have this problem.

Consider this code: https://godbolt.org/z/mHiLCm

With the options "-ffp-model=precise -fno-honor-infinities -fno-honor-nans" the math operations here are generated with the "nnan ninf contract" flags. That's correct. What will happen when I use the pragma to turn precise off? Does it enable all fast math flags? Will the subsequent "pop" leave the "ninf nnan" fast math flags enabled?

As I said, I don't think you can get into this situation with MSVC. I believe that icc will go into full fast math mode with the "precise, off, push" pragma but will go back to "nnan ninf contract" mode with the pop. At least, that's what the design docs say. I haven't looked at the code to verify this. It seems like the correct behavior in any case. I think the clang FPOptions needs individual entries for all of the fast math flags to handle this case.

clang/docs/LanguageExtensions.rst
3059

Re "fp-contraction=on": I agree that this is what it should do, but I don't think that's what fp-model=precise currently does. I think it's setting fp-contraction=fast.

3060

s/multiple/multiply

3067

Looks like you need a line break here.

3067

Are the precise and except stacks independent?

clang/include/clang/Basic/DiagnosticParseKinds.td
1111–1112

This comment is wrong after your change.

1135

This isn't quite accurate. The pop case has no comma-separated arguments. It might be better to print the full syntax here if that's feasible.

clang/include/clang/Basic/LangOptions.h
362

It seems like fp_precise describes too many things to be a single option. Even within this set of options it overlaps with fp_contract.

clang/test/CodeGen/fp-floatcontrol-stack.cpp
3

Can you add run lines for -ffast-math and (separately) "-fno-honor-nans -fno-honor-infinities"?

18

There should be a constrained fadd here also, right?

53

Why is the constrained intrinsic generated in this case? If we've got both constraints set to the defaults at the file scope I would have expected that to turn off the constrained mode.

125

Are there also fast-math flags set here? If not, why not?

mibintc marked 6 inline comments as done.Tue, Jan 28, 10:05 AM

It's not clear to me from reading this how the "precise" control is going to work with relation to the fast math flags. I don't think MSVC allows the granularity of control over these flags that we have in clang, so maybe they don't have this problem.

You're right, MSVC only allows the pragma at file scope.

Consider this code: https://godbolt.org/z/mHiLCm

With the options "-ffp-model=precise -fno-honor-infinities -fno-honor-nans" the math operations here are generated with the "nnan ninf contract" flags. That's correct. What will happen when I use the pragma to turn precise off? Does it enable all fast math flags? Will the subsequent "pop" leave the "ninf nnan" fast math flags enabled?

This patch doesn't have support for turning precise off, that's a bug, I will revisit. This is my plan for how to handle enabling/disabling fast math: IRBuilder.h has settings FMF, and also supplies clearFastMathFlags, setFastMathFlags(flags) and FastMathFlagGuard. When the expression is walked that alters the FMF, use FastMathFlagGuard to save the current state of FMF, modify the settings using the clear or set functions, walk the expression. After the expression is walked, the FMF settings will be restored to previous state by the FastMathFlagGuard destructor.

As I said, I don't think you can get into this situation with MSVC. I believe that icc will go into full fast math mode with the "precise, off, push" pragma but will go back to "nnan ninf contract" mode with the pop. At least, that's what the design docs say. I haven't looked at the code to verify this. It seems like the correct behavior in any case. I think the clang FPOptions needs individual entries for all of the fast math flags to handle this case.

Thanks I'll investigate this and add test cases. I think possibly since IRBuilder has the FMF like I described above it might work with current support. Is there currently a way to modify nan and inf at the source level or only by compiler option? BTW I've been asked to implement a pragma to control fp "reassoc" at the source level. I'm planning to do that after this patch is complete.

clang/docs/LanguageExtensions.rst
3059

Oh, I looked back at the patch for -ffp-model and precise is documented to set ffp-contract=fast. Not sure why I thought that was right. I'll have to redo it.

3067

No, the stack that tracks the float control pragma settings is a pair, roughly (IsPreciseEnabled, IsExceptEnabled)

clang/include/clang/Basic/LangOptions.h
362

I see your point. I wanted it to reflect the current pragma setting that's why I kept it intact. I'll rethink this.

clang/test/CodeGen/fp-floatcontrol-stack.cpp
3

OK. i'll add pragma's to set precise off too.

18

yes there's a constrained add following. i can add that pattern into the file check.

53

The "run" line in this case uses ffp-exception-behavior=struct; i was trying to address https://bugs.llvm.org/show_bug.cgi?id=44571 by checking the command line options to see if strict was enabled. That's why constrained intrinsics are enabled. Evidently that's incorrect.

125

that's a bug. thanks

You could check the scope where the pragma appears is the same way as pragma clang fp does. The code

case tok::annot_pragma_fp:
  ProhibitAttributes(Attrs);
  Diag(Tok, diag::err_pragma_fp_scope);
  ConsumeAnnotationToken();
  return StmtError();

is put into Parser::ParseStatementOrDeclarationAfterAttributes and it produces errors on misplaced pragma. Probably the same way may work for you.

mibintc updated this revision to Diff 243888.Tue, Feb 11, 8:50 AM

This patch is a work in progress.

The problem that I want to work on next is that the scope is wrong when the pragma token is seen following the right brace of a function body. In this patch I worked around that problem by modifying the test case to insert bogus "class ResetScope;" to force the scope back to file-level before each pragma that occurs at file scope. You can see that workaround in the test case clang/test/CodeGen/fp-floatcontrol-stack.cpp

These are the main differences with previous version of this patch:

  1. per Andy Kaylor's comments in the llvm-dev discussion "Floating Point semantic modes", I changed the -ffp-model=precise to be the default. I changed ffp-model=precise to set -ffp-contract=on [previously it was -ffp-contract=fast]. This caused the need to change some tests because the LLVM IR now has the "contract" bit enabled.
  2. I put FMF into the FPOptions struct. As I mentioned in a previous comment, I have a request to allow changing the REASSOC bit via source pragma. I plan to submit that in a separate patch. Putting FMF into FPOptions allows the fast bits to be set or disabled via pragma float_control precise, Andy had suggested this change in his code review.
  3. I put Andy's distillation of floating point options and floating point modes into UsersManual.rst
  4. I changed how the strictfp attribute was set on functions, now it's set if the strict mode is enabled in the function body e.g. either by the option level or a float_control pragma

Still needs to be done:

  1. The problem with the file scope I mentioned above
  2. I assume that the 2 denormal settings need to be added to FPOptions.
  3. Required diagnostics a la MSVC that Andy mentioned in llvm-dev discussion "Floating Point semantic modes"
  4. What else?

I would think contract change can be separated from the rest of the changes, and therefore should be a separate review (to reduce noise)?

I would think contract change can be separated from the rest of the changes, and therefore should be a separate review (to reduce noise)?

I split off that change to https://reviews.llvm.org/D74436 and added you as reviewer, thanks.

mibintc updated this revision to Diff 244894.Sun, Feb 16, 12:04 PM

I found the problem in the #pragma float_control (push/pop) stack, it was just a dumb bug.

I also added -include-pch test cases, and added code to ASTWriter ASTReader to preserve the floatcontrol pragma stack

For 2 of the new floatcontrol test cases, I temporarily added XFAIL because the patch to default -ffp-precise=fast and ffp-contract=on has been withdrawn (will re-commit soon).