Page MenuHomePhabricator

[FPEnv] Allow CompoundStmt to keep FP options
AcceptedPublic

Authored by sepavloff on Apr 18 2022, 11:04 AM.

Details

Summary

AST does not have special nodes for pragmas. Instead a pragma modifies
some state variables of Sema, which in turn result in modified
attributes of AST nodes. This technique applies to floating point
operations as well. Every AST node that can depend on FP options keep
current set of them within it.

This technique works well for options like exception behavior or fast
math options. They represent instructions to the compiler how to modify
code generation for the affected nodes. However treatment of FP control
modes has problems with this technique. Modifying FP control mode
(like rounding direction) usually requires operations on hardware, like
writing to control registers. It must be done prior to the first
operation that depends on the control mode. In particular, such
operations are required for implementation of pragma STDC FENV_ROUND,
compiler should set up necessary rounding direction at the beginning of
compound statement where the pragma occurs. As there is no representation
for pragmas in AST, the code generation becomes a complicated task in
this case.

To solve this issue FP options are kept inside CompoundStmt. Unlike to FP
options in expressions, these does not affect any operation on FP values,
but only inform the codegen about the FP options that act in the body of
the statement. As all pragmas that modify FP environment may occurs only
at the start of compound statement or at global level, such solution
works for all relevant pragmas. The options are kept as a difference
from the options in the enclosing compound statement or default options,
it helps codegen to set only changed control modes.

Diff Detail

Event Timeline

sepavloff created this revision.Apr 18 2022, 11:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
sepavloff requested review of this revision.Apr 18 2022, 11:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 11:04 AM

Thanks for working on this! One thing that's not clear to me is what bugs this is solving -- the test coverage only shows a change to textual AST dumping behavior. Is this otherwise expected to be an NFC change, or should there be some codegen tests that show a difference in behavior?

clang/include/clang/AST/Stmt.h
133

I don't think this is a bad approach, but it does further reduce the number of statements we support in a compound statement.

There's a part of me that wonders if the approach is to introduce a new AST node for a stateful compound statement; I suspect if we dig around, we'll find other pragmas that want to behave similar to floating-point feature pragmas (the idea of a pragma scoped to a block is not really new). Having something more general means we're less likely to keep stealing bits here.

clang/include/clang/Sema/ScopeInfo.h
77–83

So these are the initial FPOptions inherited by the scope from its surrounding context? And it's never updated by a pragma?

clang/lib/AST/TextNodeDumper.cpp
2375–2379

Should we have a similar change for the JSON node dumper?

sepavloff updated this revision to Diff 429525.May 15 2022, 3:13 AM

Addressed review notes

  • Added support in JSON node dumper,
  • Added support in ast-print

Thanks for working on this! One thing that's not clear to me is what bugs this is solving -- the test coverage only shows a change to textual AST dumping behavior. Is this otherwise expected to be an NFC change, or should there be some codegen tests that show a difference in behavior?

Thanks for feedback!
This change is a prerequisite for implementation of pragma STDC FENV_ROUND in D125625. FP options stored in CompoundStmt are used to set up proper rounding mode on entry to compound statement.

clang/include/clang/AST/Stmt.h
133

Possible solution is presented in D125635.
Spending a bit for state variable reduces number of bits for NumStmts to 23 bits (about 8M statements). And yes, other pragmas (or anything else) can require allocation of additional bits. As the number of statements is potentially large, it would be better to keep in an integer rather than bit-field.

clang/include/clang/Sema/ScopeInfo.h
77–83

Yes, this FPOption is used to calculate the effect of pragmas in the compound statement as a difference with FPOptions stored in Sema, so that CompoundStmt keeps only FP features that are changed in it.

clang/lib/AST/TextNodeDumper.cpp
2375–2379

Implemented.

martong removed a subscriber: martong.May 15 2022, 11:45 PM

Update after D125635 and rebase

rjmccall added inline comments.Fri, Jun 24, 7:20 AM
clang/lib/Sema/SemaStmt.cpp
452

How cheap is this? Is this something it would be worthwhile to fast-path by maintaining a bit to check for the rare case where we have an override in a scope?

sepavloff updated this revision to Diff 440241.Mon, Jun 27, 8:34 AM

Rebased, optimized FPOptions::diffWith.

sepavloff added inline comments.Mon, Jun 27, 8:37 AM
clang/lib/Sema/SemaStmt.cpp
452

Added an optimization to diffWith: detect the frequent case, when FPOptions is not changed, and implement fast-path for it.

rjmccall added inline comments.Mon, Jun 27, 8:45 AM
clang/lib/Basic/LangOptions.cpp
214

Hmm. If we can assume that all of the options are single bits (which this is doing), then this algorithm is basically FPOptions::storage_type OverrideMask = (Value ^ Base.Value);, which makes this cheap enough to be defined inline in the header.

sepavloff added inline comments.Mon, Jun 27, 10:55 AM
clang/lib/Basic/LangOptions.cpp
214

FPContractMode, ConstRoundingMode, SpecifiedExceptionMode and FPEvalMethod occupy more than one bit: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FPOptions.def.

aaron.ballman added inline comments.Mon, Jun 27, 12:17 PM
clang/include/clang/Sema/ScopeInfo.h
77–83

It might make sense to rename this to OriginalFPFeatures or InitialFPFeatures or something like that, to make it more clear that this field is not updated as we process the rest of the compound statement. WDYT?

clang/lib/AST/Stmt.cpp
370–371

There's a part of me that wonders if it's a slightly more clear design to have setStoredFPFeatures() set CompoundStmtBits.HasFPFeatures to true as needed rather than requiring the caller to do this two-step initialization process. WDYT?

clang/lib/AST/StmtPrinter.cpp
216

Why is this not handled as well and print out FE_DYNAMIC?

rjmccall added inline comments.Mon, Jun 27, 2:49 PM
clang/include/clang/Basic/LangOptions.h
749

Can you make direction more obvious in the method name? diffFrom would make it clear that the result, applied to the base, yields this.

clang/include/clang/Sema/ScopeInfo.h
77–83

Yeah, I agree; this name makes it sound like the current, live set of features, even if the comment explains that it isn't.

clang/lib/AST/Stmt.cpp
370–371

setStoredFPFeatures is only otherwise used in deserialization, and the node has to be allocated properly to support it. I think this is the right approach.

clang/lib/Basic/LangOptions.cpp
214

Oh, I see. I wasn't reasoning correctly about how this is a mask and not a value.

In non-LTO builds, we'll have to actually do this call when leaving every scope. It's a minor overhead, but it'd be nice to avoid. Please do the fast path in an inline implementation and then put this slow path out of line, in a private method named diffFromSlow or something like that.

sepavloff updated this revision to Diff 440644.Tue, Jun 28, 8:56 AM

Updated patch according to reviewers' notes

  • Rename field of CompoundScopeInfo,
  • Treat FE_DYNAMIC like other rounding mode arguments in StmtPrinter,
  • Rename and optimize method for calculation of FP option difference.
sepavloff added inline comments.Tue, Jun 28, 9:13 AM
clang/include/clang/Basic/LangOptions.h
749

I renamed this method to getChanges. I am not sure it becomes better. May be getChangesFrom or getDiffFrom? Any suggestion is welcome.

clang/include/clang/Sema/ScopeInfo.h
77–83

It makes sense. Renamed to InitialFPFeatures.

clang/lib/AST/StmtPrinter.cpp
216

Initially it was used to prevent extra FENV_ROUND FE_DYNAMIC pragma in the case when only FENV_ACCESS was specified. However after landing D126364 there is no need for that. Now FE_DYNAMIC is treated similar to other values.

clang/lib/Basic/LangOptions.cpp
214

Implemented slow and inline versions. The later is defined after definition of FPOptionsOverride in this file.

Looks good, thanks! Approved with the rename as discussed.

clang/include/clang/Basic/LangOptions.h
749

Yes, I think just adding From would make this clearer, thanks.

aaron.ballman accepted this revision.Wed, Jun 29, 6:51 AM

LGTM modulo the rename request.

clang/lib/AST/Stmt.cpp
370–371

My concern was more with new calls added later -- it seems reasonable that someone would think they could call setStoredFPFeatures() and have them stored rather than asserting the object already claims to have stored features.

That said, I don't feel strongly, so we can always adjust it in the future as new callers are added.

This revision is now accepted and ready to land.Wed, Jun 29, 6:51 AM
rjmccall added inline comments.Wed, Jun 29, 8:55 AM
clang/lib/AST/Stmt.cpp
370–371

Well, it's private, and there's a comment on it saying that it's only used by serialization. Because the trailing storage for the features doesn't actually exist unless the node was allocated specially, I think it's probably more important to allow for the safety check that the node was allocated that way than to worry about transient semi-initialized states during deserialization, which already has a lot of potential problems like this.