Page MenuHomePhabricator

Move FPFeatures from BinaryOperator bitfields to Trailing storage
Needs ReviewPublic

Authored by mibintc on Wed, Mar 18, 1:20 PM.

Details

Summary

This patch moves the FPFeatures field from BinaryOperator bitfields to Trailing storage, since there is a current desire to widen the FPOptions type. It's a preliminary patch, looking for feedback from @rjmccall and others. On the dev email list, there was an alternate proposal to maintain this information in state. I had already partly implemented this change so I just continued along this way.

There's still a problem in the patch, there's a bug in the AST statement visitor, something wrong with the template metaprogramming? i still haven't found that problem, but i wanted to put this up for any feedback before i find and fix that problem, If you see what i did wrong please let me know. The problem has to do with combining the CompoundAssignmentOperator and BinaryOperator. I needed to do that because BinaryOperator needs to be finalized before Trailing storage can be used.

FPFeatures are also needed on CXXOperatorCall. I put that field into CallExpr. Do you want that to go into Trailing storage as well? I didn't do that. It would also require CallExpr and CXXOperatorCall to be collapsed together.

Diff Detail

Unit TestsFailed

TimeTest
960 msClang Tools.clang-tidy/checkers::google-runtime-int.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/clang-tidy/checkers/google-runtime-int.cpp google-runtime-int /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/google-runtime-int.cpp.tmp
2,980 msClang.Analysis::ctor.mm
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -triple i386-apple-darwin10 -DI386 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify -analyzer-config eagerly-assume=false /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/Analysis/ctor.mm
2,500 msClang.Analysis::operator-calls.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -std=c++11 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/Analysis/operator-calls.cpp
2,240 msClang.CXX/special/class_copy::implicit-move-def.cpp
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -emit-llvm -triple x86_64-unknown-linux-gnu -o - -std=c++11 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CXX/special/class.copy/implicit-move-def.cpp | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck -check-prefix=CHECK-ASSIGN /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CXX/special/class.copy/implicit-move-def.cpp
2,230 msClang.CodeGenCXX::constructor-init.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -std=c++11 -triple x86_64-apple-darwin10 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/constructor-init.cpp -emit-llvm -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/test/CodeGenCXX/Output/constructor-init.cpp.tmp
View Full Test Results (10 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald Transcript

There's a bug in this patch that i haven't solved yet. I think it's just 1 bug. The bug is that the AST statement visitor is going to the "fallback" instead of the right destination when walking CompoundAssignmentOperator. This patch collapses CompoundAssignmentOperator class into BinaryOperator class. In a spot check, i think all the failing tests are because the Visitor walk isn't working correctly for CompoundAssign (for example += ). These are the test fails reported by check-clang in my sandbox,
Failing Tests (15):

Clang :: Analysis/loopexit-cfg-output.cpp
Clang :: CXX/drs/dr2xx.cpp
Clang :: CXX/expr/expr.const/p2-0x.cpp
Clang :: CXX/expr/expr.prim/expr.prim.lambda/expr.prim.lambda.capture/p17.cpp
Clang :: CodeGen/ffp-contract-fast-option.cpp
Clang :: CodeGenCXX/const-init-cxx1y.cpp
Clang :: CodeGenCXX/const-init-cxx2a.cpp
Clang :: CodeGenCXX/non-const-init-cxx2a.cpp
Clang :: Sema/warn-unreachable.c
Clang :: Sema/warn-unsequenced.c
Clang :: SemaCXX/constant-expression-cxx1y.cpp
Clang :: SemaCXX/constant-expression-cxx2a.cpp
Clang :: SemaCXX/decomposed-condition.cpp
Clang :: SemaCXX/integer-overflow.cpp
Clang :: SemaCXX/warn-unsequenced.cpp
martong removed a subscriber: martong.Thu, Mar 19, 2:57 AM
mibintc updated this revision to Diff 251699.Fri, Mar 20, 10:40 AM
mibintc retitled this revision from Move FPFeatures from BinaryOperator bitfields to Trailing storage - preliminary to Move FPFeatures from BinaryOperator bitfields to Trailing storage.

This revision has been rebased, the LIT test failures seen in the previous revision have been corrected: check-all passes. Still owe you some polishing. I'm going to add a couple inline comments.

mibintc marked 3 inline comments as done.Fri, Mar 20, 10:49 AM

There's a bug in this patch that i haven't solved yet. I think it's just 1 bug. The bug is that the AST statement visitor is going to the "fallback" instead of the right destination when walking CompoundAssignmentOperator. This patch collapses CompoundAssignmentOperator class into BinaryOperator class. In a spot check, i think all the failing tests are because the Visitor walk isn't working correctly for CompoundAssign (for example += ). These are the test fails reported by check-clang in my sandbox,
Failing Tests (15):

Clang :: Analysis/loopexit-cfg-output.cpp
Clang :: CXX/drs/dr2xx.cpp
Clang :: CXX/expr/expr.const/p2-0x.cpp
Clang :: CXX/expr/expr.prim/expr.prim.lambda/expr.prim.lambda.capture/p17.cpp
Clang :: CodeGen/ffp-contract-fast-option.cpp
Clang :: CodeGenCXX/const-init-cxx1y.cpp
Clang :: CodeGenCXX/const-init-cxx2a.cpp
Clang :: CodeGenCXX/non-const-init-cxx2a.cpp
Clang :: Sema/warn-unreachable.c
Clang :: Sema/warn-unsequenced.c
Clang :: SemaCXX/constant-expression-cxx1y.cpp
Clang :: SemaCXX/constant-expression-cxx2a.cpp
Clang :: SemaCXX/decomposed-condition.cpp
Clang :: SemaCXX/integer-overflow.cpp
Clang :: SemaCXX/warn-unsequenced.cpp

The new revision corrects all these fails. i have a couple questions about test cases, i've inline those in the test case modification. Also need to do clang-format and also some changes to CXXCallOperator to remove redundant accessor

clang/include/clang/AST/ExprCXX.h
168

i can get rid of these 2 accessor functions, we get them from CallExpr

clang/test/Analysis/loopexit-cfg-output.cpp
211 ↗(On Diff #251699)

With these changes, the blocks print out in a different order but the semantic meaning appears to be the same. Is this difference acceptable?

clang/test/Sema/warn-unreachable.c
86 ↗(On Diff #251699)

The srcpos for compound assignment is now the same as srcpos for ordinary binary operator, so i changed the test case to move the diagnostic, ok?

mibintc updated this revision to Diff 251746.Fri, Mar 20, 1:28 PM

This revision fixes the clang-tidy,clang-format and removes the redundant accessors. Ready for your review.

Let's make this patch be purely a representation change. I wouldn't expect *any* test changes from that; if there are, we should understand why. You can then add the stuff about tracking whether there's a pragma in a separate patch.

clang/include/clang/AST/StmtVisitor.h
92

Let's stick with the existing formatting for consistency. It's just vertically aligning the DISPATCH and class-name tokens to emphasize the common pattern.

clang/include/clang/Serialization/ASTBitCodes.h
1566

I think the intent is that we don't change constant values in this file. You can leave this in place with a comment saying it's defunct, though.

Let's make this patch be purely a representation change. I wouldn't expect *any* test changes from that; if there are, we should understand why. You can then add the stuff about tracking whether there's a pragma in a separate patch.

OK, some of the tests show ConditionalOperator in the IR matching. Since that operator is necessarily getting merged into BinaryOperator those tests will have to change. I'll see about changing the srcpos for the conditional assignment operator so i can undo the srcpos changes in those tests.

Let's make this patch be purely a representation change. I wouldn't expect *any* test changes from that; if there are, we should understand why. You can then add the stuff about tracking whether there's a pragma in a separate patch.

OK, some of the tests show ConditionalOperator in the IR matching. Since that operator is necessarily getting merged into BinaryOperator those tests will have to change. I'll see about changing the srcpos for the conditional assignment operator so i can undo the srcpos changes in those tests.

I assume you mean CompoundAssignOperator. Sure, if there's some sort of debug output / AST dump in a test that includes expression node names, obviously that's okay to change; and it's possible that the new behavior with CompoundAssignOperator not split out is better in some way. We should just make sure we understand any changes we see.

mibintc updated this revision to Diff 252469.Tue, Mar 24, 6:43 PM

I think this addresses all the feedback from @rjmccall

This comment was removed by mibintc.

I don't see those Harbormaster fails, using trunk and RHEL8 on architecture x86_64, Debug build of clang;clang-tools-extra and check-all. I ran all the failed lit tests separately and each passed

rjmccall added inline comments.Wed, Mar 25, 9:07 AM
clang/include/clang/AST/Expr.h
2638

Let's split the CallExpr changes out into a separate patch, so that we have one patch that's *purely* about unifying BinaryOperator and CompoundAssignOperator and putting FPOptions in trailing storage, and then another that's about adding FPOptions to CallExpr.

For that latter patch, CallExpr already has its own, hand-rolled concept of trailing storage which you should be able to emulate instead of unifying the hierarchy.

3467

Why does this use unsigned for the trailing storage of the FPOptions?

3471

Can these bits go in the bitfields?

3521

Okay, so this is *always* adding trailing storage. Is there a common value for FPOptions — something that corresponds to default settings, for example — that we could use to avoid needing storage in the common case?

Also, it would be useful to extract a function somewhere that you can use in all of these places for consistency, maybe something like this on FPOptions:

/// Does this FPOptions require trailing storage when stored in various AST nodes,
/// or can it be recreated using `defaultWithoutTrailingStorage`?
bool requiresTrailingStorage() const { return *this == defaultWithoutTrailingStorage(); }

/// Return the default value of FPOptions that's used when trailing storage isn't required.
static FPOptions defaultWithoutTrailingStorage() { ... }
3527

Please change the text in this assertion and the one in the constructor above. The requirement is now that you use the appropriate constructor for the case. Or maybe we should just have one constructor that takes optional CompLHS/CompResult types and then assert that they're given if we're building a compound assignment? If you do the same for Create, the whole thing might end up being much more convenient for callers, too.

3679

Are these necessary?

3708

These should all assert that they're being used on a compound-assignment operator.

Please preserve the comments from CompoundAssignOperator on these.

6080

Why is this in this patch?

clang/lib/AST/Expr.cpp
4358

These places should use that same unified predicate.

mibintc marked an inline comment as done.Wed, Mar 25, 9:18 AM
mibintc added inline comments.
clang/include/clang/AST/Expr.h
3521

The reason I set hasFPFeatures to True in this revision is because in the previous review you asked me to make it purely a representational change and "add stuff about whether there is a pragma in a separate patch". The stuff I had in the previous version was smarter about creating trailing storage, it only created trailing storage when the pragma was in effect. I envirsioned that the evolution would accept something that always created the FPOptions in trailing storage, and in the 2nd generation would adopt the more thrifty way of creating trailing storage.

rjmccall added inline comments.Wed, Mar 25, 9:56 AM
clang/include/clang/AST/Expr.h
3521

Well, let's at least set up the infrastructure and APIs correctly, even if we always allocate trailing storage.

For example, should getFPOptions take an ASTContext & so that it can always return the right options instead of making clients do hasFPOptions() ? getFPOptions() : ctx.getFPOptions()?

mibintc updated this revision to Diff 253341.Sat, Mar 28, 6:56 AM
mibintc marked 12 inline comments as done.

@rjmccall Many thanks for your code review. Please take a look at this when you get a chance.

mibintc marked an inline comment as done.Sat, Mar 28, 6:59 AM

I added some inline replies to John. I'm not certain I have everything yet exactly the way he wants it.

clang/include/clang/AST/Expr.h
2638

I've split off the CallExpr changes to a future patch - will you give me a hint about CallExpr's own hand-rolled concept of trailing storage? e.g. a line number or identifier name that i can hunt for and track down.

3467

I will change it to FPOptions

3470

I think I need hasFPFeatures in the node itself because that's how i know whether there's trailingstorage e.g. in the AST reader-writer. hasFPFeatures is always true at the moment but that will be improved in the next generation.

3471

i eliminated isCompoundAssign because, i can just check the opcode, i think there's always a opcode or a dummy opcode. But I need hasFPFeatures

3521

To explain further, in the pragma patch, which is fully formed but split off for a future commit, the Sema structure holds the current value of the floating point settings (in FPOptions FPFeatures). It is initialized from the command line, and as compound statements/blocks which contain pragma's are entered and exited, the value of Sema.FPFeatures is updated. I will add a bit "has_pragma_features" to FPOptions. When that bit is true, I know that the initial value of FPOptions (which is derived from command line) is different than the current settings. In this circumstance only it is necessary to hold FPFeatures in the Expr nodes (in Trailing storage). In all other cases, FPOptions can be derived from LangOpts and LangOpts is available in each clang pass. There are a bunch of occurrences where FPOptions() has been added as an nth parameter to various function calls, i believe though I'm not certain that in those cases the value of FPOptions is "don't care". (those occurrences were pre-existing in the source code). I hope this makes sense.

3527

I combined the dual Constructor's and Create's into singles with default parameters. I had to do some juggling at some of the call sites to make the new interface work. Do you prefer this way?

6080

I'm not sure what this is referring to?

clang/include/clang/AST/StmtVisitor.h
92

You had mentioned in your previous review about having things lined up properly here, and I don't disagree--this comment is just to let you know that I had applied the clang-format patch that was supplied by the BuildBots in the pre-commit checks. The buildbot pre-commit check wants them on different lines. I've switched the formatting to what you request.

clang/include/clang/Basic/LangOptions.h
381

I added these at your suggestion but not presently making use of them

clang/lib/AST/Expr.cpp
4358

I'm not sure if I captured what you want here.

On my RHEL8 system building Debug clang;clang-tools-extra, check-all is passing.

rjmccall added inline comments.Sat, Mar 28, 12:50 PM
clang/include/clang/AST/Expr.h
2638

Sure, check out CallExpr's sizeOfTrailingObjects and offsetToTrailingObjects.

3470

It's pretty common to make ASTStmtReader and ASTStmtWriter friends of the bitfields struct.

3471

If IsCompoundAssign is important enough to be worth optimizing, it's not a problem per se to keep it separate. Up to you. But it can definitely go in the bitfields if you need it.

3521

That does make sense, thank you. I wonder if it would be sensible for Sema to maintain not just the current state + whether there are any pragma overrides but also an idea of what specifically has been overridden. That would make the AST basically agnostic to the default settings; you'd always be able to just apply the overrides to the defaults. At that point, we'd be able to re-use serialized modules in builds with different fast-math settings, which would be really nice.

On the other hand, since the default settings are sometimes exposed as #defines (e.g. FAST_MATH) and therefore can influence the AST in other ways, maybe we wouldn't be able to re-use serialized modules that way.

Regardless, the right API would be to pass in the ASTContext so that this accessor can simply return the correct settings instead of making the caller merge information.

3527

QualType has an empty state, so I would just declare the parameters like QualType CompLHSType = QualType(). That'll make the call sites much simpler, because you can structure the code like:

QualType CompLHSType, CompResultType;
if (E->isCompoundAssign()) {
  CompLHSType = ...;
  CompResultType = ...;
}
return BinaryOperator::Create(...);
3669

We generally lower-case method names, so these would be isCompoundAssign() and hasFPFeatures() (maybe hasFPFeatureOverrides()?).

6080

I was probably looking at the wrong diff, sorry. The "new changes only" display can get confused when patches are rebased.

clang/include/clang/Basic/LangOptions.h
381

Please do. You don't have to do it how I suggested — if you want to make requiresTrailingStorage just return true, that's fine — but please use this to centralize all the decisions about whether or not trailing storage is required.

mibintc updated this revision to Diff 253983.Tue, Mar 31, 1:32 PM

Here's another revision, I believe this handles all of @rjmccall 's requests. Thank you, John, for your review!

rjmccall added inline comments.Tue, Mar 31, 4:25 PM
clang/include/clang/AST/Expr.h
3474

Sorry, I wasn't trying to say you need this here! Since you can use TrailingObjects for BinaryOperator, you absolutely should take advantage of what it does. I was just pointing you at the CallExpr stuff so that you can see the places you'll need to update when you add this storage to CallExpr.

3679

I'm not sure it's a good idea to have this accessor, at least not with such a tempting name. Maybe getStoredFPFeatures() and hasStoredFPFeatures()?

3695

These two accessors will need to take ASTContexts eventually. Are you planning to do that in a follow-up patch?

clang/include/clang/AST/JSONNodeDumper.h
265

What actually calls this? StmtVisitor as written doesn't fall back on a VisitCompoundAssignOperator. You could *make* it fall back on one, of course.

clang/include/clang/AST/RecursiveASTVisitor.h
427

Comment needs updating.

clang/include/clang/AST/StmtVisitor.h
146

Comment needs updating, but more importantly, you're making all of these fall back on VisitBinAssign, which is definitely not correct.

clang/include/clang/Basic/LangOptions.h
394

The problem with having both functions that take ASTContexts and functions that don't is that it's easy to mix them, so they either need to have the same behavior (in which case it's pointless to have an overload that takes the ASTContext) or you're making something really error-prone.

I would feel a lot more confident that you were designing and using these APIs correctly if you actually took advantage of the ability to not store trailing FPOptions in some case, like when they match the global settings in the ASTContext. That way you'll actually be verifying that everything behaves correctly if nodes don't store FPOptions. If you do that, I think you'll see my point about not having all these easily-confusable functions that do or do not take ASTContexts..

Trying to help Melanie respond to the comments, so I ran through the patch and came up with the following comments/responses. Sorry for the 'surprise hit' :)

clang/include/clang/AST/Expr.h
3474

Yep, this shouldn't be necessary. Uses of this should be able to use totalSizeToAlloc and additionalSizeToAlloc.

3674

Since changing this value can result in a change of allocation size, I don't think this should be settable after creation.

3680

Whats the purpose of having both of these? It seems that you can either: 1- Have only the first and require consumers to check if they aren't sure, or 2- Only do the second, and those who have checked just get the default anyway.

3739

Nit, I'd prefer this be named 'isCompoundAssignment'. Also, the patch seems a little inconsistent with FPFeatures vs FPOptions.

clang/include/clang/AST/StmtVisitor.h
146

@rjmccall : What would you think fits better? If we don't have something like this, VisitBinaryOperator is going to have to redirect to VisitBinAssign anyway. We could presumably keep VisitCompoundAssignOperator, but that seems like a naming difference.

clang/include/clang/Basic/LangOptions.h
385

Nit: return {}; ?

Also, why are there two versions of this? It seems that the ASTContext one should be the only required one.

394

I think I disagree with @rjmccall that these requiresTrailingStorage should be here at all. I think we should store in the AST ANY programmer opinion, even if they match the global setting. It seems to me that this would be more tolerant of any global-setting rewrites that modules/et-al introduce, as well as make the AST Print consistent. Always storing FPOptions when the user has explicitly overriding it also better captures the programmer's intent.

clang/lib/AST/Expr.cpp
4347

Again, should just use the TrailingObjects::totalSizeToAlloc I think. Alternatively, implement a new 'totalSizeToAlloc' based on the hasFPFeatures and isCompound).

clang/lib/AST/ExprConstant.cpp
7788

Perhaps not changing this name is a good idea, and just leaving this as VisitCompoundOperator, then having the 'default' behavior to be to just call VisitBinaryOperator.

rjmccall added inline comments.Wed, Apr 1, 9:57 AM
clang/include/clang/AST/StmtVisitor.h
146

VisitBinAssign is an existing method that is used specifically for the simple assignment operator. The old delegation was that VisitBinMulAssign delegated to VisitCompoundAssignOperator, which delegated to VisitBinaryOperator (because CAO was a subclass of that), which delegated to VisitExpr. The natural new delegation here would be for VisitBinMulAssign to just delegate directly to VisitBinaryOperator. If it's still useful to have an intermediate visitor method in the delegation chain for all the compound assignment operators, that's fine, but it shouldn't be VisitBinAssign.

clang/include/clang/Basic/LangOptions.h
394

I covered this elsewhere in the review. If you want to have that tolerance — and I think you should — then expressions should store (and Sema should track) the active pragma state, which can be most easily expressed as a pair of an FPOptions and a mask to apply to the global FPOptions. When you enter a pragma, you clear the relevant bits from the global FPOptions mask.

But the whole point of putting this stuff in trailing storage is so that you can make FPOptions as big as you need without actually inflating the AST size for a million nodes that don't care in the slightest about FPOptions.

rjmccall added inline comments.Wed, Apr 1, 10:08 AM
clang/include/clang/Basic/LangOptions.h
394

But the whole point of putting this stuff in trailing storage is so that you can make FPOptions as big as you need without actually inflating the AST size for a million nodes that don't care in the slightest about FPOptions.

I meant to say: for a million nodes that don't care in the slightest about FPOptions, as well as for a million more nodes that aren't using pragma overrides.

erichkeane added inline comments.Wed, Apr 1, 10:13 AM
clang/include/clang/Basic/LangOptions.h
394

Right, I get the intent, and I completely agree with that. My point was EVERY Expr that is affected by a #pragma should store it. Though, after looking at your Macro concern above, I'm less compelled.

I guess was suggesting that the logic for "requiresTrailingStorage" should just be "modified by a pragma" instead of "FPOptions != The global setting".

rjmccall added inline comments.Wed, Apr 1, 11:31 AM
clang/include/clang/Basic/LangOptions.h
394

Well, "modified by a pragma" still wouldn't make the AST agnostic to global settings, since the pragmas don't override everything in FPOptions at once. But I agree that would achieve the most important goal, which is to stop inflating the AST when pragmas *aren't* in effect, which is approximately 100% of the time. In order to do that, though, we'll need to start tracking pragmas, which we should do but which can wait for a follow-up patch. In the meantime, I don't think you're ever going to get the interfaces right for queries like BinaryOperator::getFPOptions unless you actually stop relying on the fact that you're unconditionally storing FPOptions. You need to passing around ASTContexts for that. That's why I'm suggesting using an exact match with the global settings as a simple thing you can easily check without modifying what data you collect in FPOptions.

erichkeane added inline comments.Wed, Apr 1, 11:34 AM
clang/include/clang/Basic/LangOptions.h
394

That sounds like a good plan to me. Thanks for entertaining my conversation/questions.

sepavloff added inline comments.Thu, Apr 2, 5:06 AM
clang/include/clang/Basic/LangOptions.h
394

we'll need to start tracking pragmas

This is made in D76599 by representing floating point pragmas with a special statement node. These nodes allow an AST consumer like CodeGen or constant evaluator to maintain current set of floating options when it traverses AST. This approach looks simpler and more consistent as global state is represented as a variable in AST consumer and is not replicated to every relevant node. It makes easier to implement codegen for things like rounding mode, when change of the FP state requires specific instructions. A pragma statement can be used to generate required code. But if the state is spread by several nodes, it is more difficult for codegen to create necessary prolog/epilog code. Now compiler does not have support of properties that need synchronization with hardware, so these problems are not topical yet, but they eventually will arise.

rjmccall added inline comments.Thu, Apr 2, 9:00 AM
clang/include/clang/Basic/LangOptions.h
394

Constant evaluation does not normally traverse the AST in the way you mean. It does this when evaluating a constexpr function, but that's not the dominant case of constant evaluation.

At the LLVM level, I think inlining, reordering, and ABI requirements on calls argue against a simple implementation model based on setting hardware flags when a pragma is entered and resetting them on exit.