This is an archive of the discontinued LLVM Phabricator instance.

Move FPFeatures from BinaryOperator bitfields to Trailing storage
ClosedPublic

Authored by mibintc on Mar 18 2020, 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

Event Timeline

mibintc created this revision.Mar 18 2020, 1:20 PM
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.Mar 19 2020, 2:57 AM
mibintc updated this revision to Diff 251699.Mar 20 2020, 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.Mar 20 2020, 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.Mar 20 2020, 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 ↗(On Diff #251746)

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
1560 ↗(On Diff #251746)

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.Mar 24 2020, 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.Mar 25 2020, 9:07 AM
clang/include/clang/AST/Expr.h
2644

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.

3473

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

3475

Can these bits go in the bitfields?

3522

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() { ... }
3528

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.

3699

Are these necessary?

3710

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

Please preserve the comments from CompoundAssignOperator on these.

6116

Why is this in this patch?

clang/lib/AST/Expr.cpp
4365

These places should use that same unified predicate.

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

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.Mar 25 2020, 9:56 AM
clang/include/clang/AST/Expr.h
3522

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.Mar 28 2020, 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.Mar 28 2020, 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
2644

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.

3473

I will change it to FPOptions

3474

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.

3475

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

3522

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.

3528

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?

6116

I'm not sure what this is referring to?

clang/include/clang/AST/StmtVisitor.h
92 ↗(On Diff #251746)

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
405

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

clang/lib/AST/Expr.cpp
4365

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.Mar 28 2020, 12:50 PM
clang/include/clang/AST/Expr.h
2644

Sure, check out CallExpr's sizeOfTrailingObjects and offsetToTrailingObjects.

3474

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

3475

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.

3522

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.

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

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(...);
3689

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

6116

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
405

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.Mar 31 2020, 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.Mar 31 2020, 4:25 PM
clang/include/clang/AST/Expr.h
3478

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.

3681

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–3697

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 ↗(On Diff #253983)

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 ↗(On Diff #253983)

Comment needs updating.

clang/include/clang/AST/StmtVisitor.h
146 ↗(On Diff #253983)

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
418

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
3478

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

3682

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.

3694

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

3775

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 ↗(On Diff #253983)

@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
409

Nit: return {}; ?

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

418

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
4354

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 ↗(On Diff #253983)

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.Apr 1 2020, 9:57 AM
clang/include/clang/AST/StmtVisitor.h
146 ↗(On Diff #253983)

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
418

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.Apr 1 2020, 10:08 AM
clang/include/clang/Basic/LangOptions.h
418

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.Apr 1 2020, 10:13 AM
clang/include/clang/Basic/LangOptions.h
418

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.Apr 1 2020, 11:31 AM
clang/include/clang/Basic/LangOptions.h
418

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.Apr 1 2020, 11:34 AM
clang/include/clang/Basic/LangOptions.h
418

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

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

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.Apr 2 2020, 9:00 AM
clang/include/clang/Basic/LangOptions.h
418

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.

sepavloff added inline comments.Apr 3 2020, 10:35 AM
clang/include/clang/Basic/LangOptions.h
418

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.

Evaluate* functions accept EvalInfo as argument, it can be extended to contain the current FPOptions, taken from Sema.

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.

For targets that encode FP environment in instructions this is true. But most targets encode FP environment in hardware registers, and a model, in which required FP environment is set at entry to some region and reset on exit from it, is very attractive. Actually constrained intrinsics is a way to prevent from reordering and similar optimizations that break this simple model. As C language provide setting FP environment only at block (or global) level it would be natural if AST would have similar property.

rjmccall added inline comments.Apr 3 2020, 11:10 AM
clang/include/clang/Basic/LangOptions.h
418

Many clients of the constant evaluator are not tied to Sema or are analyzing code that wasn't necessarily written in the current context. What you're discussing is *extremely* error-prone.

For targets that encode FP environment in instructions this is true. But most targets encode FP environment in hardware registers, and a model, in which required FP environment is set at entry to some region and reset on exit from it, is very attractive.

The constrained-intrinsics representation records options on each intrinsic, so no, it wouldn't naturally support a setup where the frontend emits intrinsics that change hardware flags on entry/exit to a region. That nicely matches a model where options are set on each expression. It also, fortunately, naturally supports optimizations like inlining as long as we turn non-intrinsic operations into intrinsics where necessary.

mibintc updated this revision to Diff 255301.Apr 6 2020, 5:46 AM

I beleive this patch responds to all @rjmccall 's review comments, except I don't know how to program a solution to his StmtVisitor remark. I'll add more info about that. This patch optionally allocates the trailing storage in BinaryOperator. I verified that it was optionally allocated by adding an assert when trailing storage was allocated; the only lit tests that failed were the floating point pragma tests.

If I change StmtVisitor to send compound assignment opcodes to BinaryOperator, then 12 clang LIT tests fail, all related to compound assignment mishandling, i.e. this patch

  • a/clang/include/clang/AST/StmtVisitor.h

+++ b/clang/include/clang/AST/StmtVisitor.h
@@ -143,7 +143,7 @@ public:

// methods, fall back on VisitCompoundAssignOperator.

#define CAO_FALLBACK(NAME) \

RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) {                 \
  • DISPATCH(BinAssign, BinaryOperator); \

+ DISPATCH(BinaryOperator, BinaryOperator); \

}
CAO_FALLBACK(MulAssign) CAO_FALLBACK(DivAssign) CAO_FALLBACK(RemAssign)
CAO_FALLBACK(AddAssign) CAO_FALLBACK(SubAssign) CAO_FALLBACK(ShlAssign)

These clang lit tests fail,
Failing Tests (12):

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 :: CodeGenCXX/const-init-cxx1y.cpp
Clang :: CodeGenCXX/const-init-cxx2a.cpp
Clang :: CodeGenCXX/non-const-init-cxx2a.cpp
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
sepavloff added inline comments.Apr 6 2020, 6:10 AM
clang/include/clang/Basic/LangOptions.h
418

Many clients of the constant evaluator are not tied to Sema or are analyzing code that wasn't necessarily written in the current context. What you're discussing is *extremely* error-prone.

There is another implementation of representing FP environment in AST: D77545. Although it is made for limited number of cases, it could be extended.

It seems like the macro's in StmtVisitor.h only work if CAO and BinaryOperator are separate classes.

I would guess that some visitor still has a VisitCompoundAssignOperator implementation, and that the behavior of VisitBinAssign happens to do what's needed for it.

You basically have two options:

  • Keep a VisitCompoundAssignOperator around in StmtVisitor for all the compound-assignment operators to delegate to. You'll have to implement its delegation to VisitBinaryOperator manually.
  • Search the source base for any remaining implementations of VisitCompoundAssignOperator and fold them into VisitBinaryOperator.
mibintc updated this revision to Diff 256648.Apr 10 2020, 1:42 PM

I finally decided that combining BinaryOperator and CompoundAssignOperator was too difficult, this patch uses the trailing object approach similar to that used in CallExpr. @rjmccall thank you once again for all your reviews in developing this patch. I hope that this version is acceptable or within a hair's breadth of acceptable. As an experiment to confirm to myself that the trailing storage was only active in the pragma case, i added a "assert(0)" to getStoredFPFeatures. In that case check-clang showed 5 expected fails for the pragma tests cases. ( Clang :: CodeGen/constrained-math-builtins.c Clang :: CodeGen/fp-contract-fast-pragma.cpp Clang :: CodeGen/fp-contract-on-asm.c Clang :: CodeGen/fp-contract-on-pragma.cpp Clang :: CodeGen/fp-contract-pragma.cpp)

Then i removed that assert before submitting this to review.

Okay, that's fine, it's definitely a simpler change to not mess with the basic node hierarchy. The one problem I see is that you've (accidentally?) set up the FPOptions methods so that you can't call them on a BinaryOperator that's actually a CompoundAssignOperator — they'll end up accessing the wrong memory. Fortunately, that's pretty easy to fix, and fixing it should let you drop all the duplicate methods you've currently got on CompoundAssignOperator.

clang/include/clang/AST/Expr.h
3486

You should add an offsetOfTrailingStorage() method that returns isa<CompoundAssignOperator>(this) ? sizeof(CompoundAssignOperator) : sizeof(BinaryOperator), and then you can add that here instead of hard-coding sizeof(BinaryOperator).

mibintc updated this revision to Diff 256998.Apr 13 2020, 8:55 AM

I made the changes requested by @rjmccall ; I also used clang-format on the tip. check-clang is passing. Look OK?

Thanks. Just a bunch of fairly minor requests now.

clang/include/clang/AST/Expr.h
3480

You don't need the non-const overload.

3723

Should HasFPFeatures be a bool everywhere you're passing it around?

clang/lib/AST/Expr.cpp
4402

Please define this in the header. You can just do:

inline size_t BinaryOperator::offsetOfTrailingStorage() const { ... }

after both of the classes are fully defined.

clang/lib/Analysis/BodyFarm.cpp
120

These construction sites don't seem like appropriate uses of defaultWithoutTrailingStorage, which is an implementation detail of the AST nodes. The right behavior here is for all of these places to use the default FPOptions from the language options, then let the AST figure out the right way to store that. That happens to have the same effect currently as defaultWithoutTrailingStorage, but the latter should be able to change, while the right behavior for these places will remain the same.

mibintc updated this revision to Diff 257055.Apr 13 2020, 12:06 PM

Responded to @rjmccall 's review

mibintc marked 3 inline comments as done.Apr 13 2020, 12:10 PM

Adding an inline reply for John. rebased the patch, also re-applied clang-format. What do you think?

clang/lib/Analysis/BodyFarm.cpp
120

OK i changed these in BodyFarm to use the default constructor. In Sema I changed them from defaultWithoutTrailingStorage to use Sema.FPFeatures which is the current setting combining pragma values + command line options.

rjmccall added inline comments.Apr 13 2020, 1:05 PM
clang/lib/Analysis/BodyFarm.cpp
120

That's right for Sema. For all the other generated uses, I think you should use FPOptions(C.getLangOpts()) instead of the default constructor. Most of these don't really care about FPOptions — they're just creating an assignment or some other operation which is not FP-sensitive — but for the ones that do, we should follow the default behavior in the language options.

mibintc updated this revision to Diff 257704.Apr 15 2020, 6:46 AM

Lost power in Monday's storm, back online today. I made the changes requested by @rjmccall. Look OK?

Minor fixes, then LGTM.

clang/lib/AST/ASTImporter.cpp
6822

Oh, these two calls need to use E->getFPFeatures(Importer.getFromContext()), since that's the context that the node was built in.

clang/lib/CodeGen/CGObjC.cpp
1497

Missed this one.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2935

Missed this one.

clang/lib/Sema/SemaOverload.cpp
13784

This should use FPFeatures, I think.

mibintc updated this revision to Diff 257789.Apr 15 2020, 11:58 AM

Responding to @rjmccall 's review. John, after this is approved I want to proceed with pragma float_control as proposed in D72841. Can you recommend an approach, do you think I will need to do it incrementally?

Responding to @rjmccall 's review. John, after this is approved I want to proceed with pragma float_control as proposed in D72841. Can you recommend an approach, do you think I will need to do it incrementally?

I think the basic technical approach from that patch seems reasonable. It's not my goal to hold up your work to pursue other technical goals; we took this detour just to minimize the memory impact of your work.

I *would* like an NFC patch first that renames FPFeatures to something like CurFPFeatures in order to more clearly distinguish it from FPOptions in Sema code, though. That should be very quick.

rjmccall accepted this revision.Apr 15 2020, 12:15 PM

Oh, and this patch LGTM, thank you.

This revision is now accepted and ready to land.Apr 15 2020, 12:15 PM

Responding to @rjmccall 's review. John, after this is approved I want to proceed with pragma float_control as proposed in D72841. Can you recommend an approach, do you think I will need to do it incrementally?

I think the basic technical approach from that patch seems reasonable. It's not my goal to hold up your work to pursue other technical goals; we took this detour just to minimize the memory impact of your work.

Thanks for all. I'm sure my work will be easier in the long run to have this well scrutinized. And I learned a lot. And I endorse not being wasteful of memory.

I *would* like an NFC patch first that renames FPFeatures to something like CurFPFeatures in order to more clearly distinguish it from FPOptions in Sema code, though. That should be very quick.

OK. I think this means I can commit now and change the identifier spelling in a future standalone patch.

I *would* like an NFC patch first that renames FPFeatures to something like CurFPFeatures in order to more clearly distinguish it from FPOptions in Sema code, though. That should be very quick.

You mean, Sema.FPFeatures -> Sema.CurFPFeatures, confirm?

This revision was automatically updated to reflect the committed changes.

I *would* like an NFC patch first that renames FPFeatures to something like CurFPFeatures in order to more clearly distinguish it from FPOptions in Sema code, though. That should be very quick.

You mean, Sema.FPFeatures -> Sema.CurFPFeatures, confirm?

Right.

@rjmccall Can you check the patch added last night here, commit 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf
Author: Benjamin Kramer <benny.kra@googlemail.com>
Date: Thu Apr 16 11:45:02 2020 +0200

LangOptions cannot depend on ASTContext, make it not use ASTContext directly

Fixes a layering violation introduced in 2ba4e3a4598b165245c581c506a813cd4a7dce33.
martong added inline comments.Apr 16 2020, 7:10 AM
clang/lib/AST/ASTImporter.cpp
6821

This introduced an assertion failure during CTU analysis. The reason is that the LHSType and the ResultType have been imported twice.

The fix is in e033ec291a1b72f307ab14569ca99822c127610b

Details:

   clang: ../../git/llvm-project/clang/lib/Basic/SourceManager.cpp:918: clang::FileID clang::SourceManager::getFileIDLoaded(unsigned int) const: Assertion `0 && "Invalid SLocOffset or bad function choice"' failed.
   clang::SourceManager::getDecomposedExpansionLoc(clang::SourceLocation) const
   clang::SourceManager::getPresumedLoc(clang::SourceLocation, bool) const
   clang::ASTImporter::Import(clang::SourceLocation)
   llvm::Error clang::ASTImporter::importInto<clang::SourceLocation>(clang::SourceLocation&, clang::SourceLocation const&)
   clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&)
   clang::ASTNodeImporter::VisitRecordDecl(clang::RecordDecl*)
   clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(clang::Decl*)
   clang::ASTImporter::Import(clang::Decl*)
   clang::ASTNodeImporter::VisitRecordType(clang::RecordType const*)
   clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*)
   clang::ASTImporter::Import(clang::QualType)
   clang::ASTNodeImporter::VisitElaboratedType(clang::ElaboratedType const*)
   clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*)
   clang::ASTImporter::Import(clang::QualType)
   clang::ASTNodeImporter::VisitPointerType(clang::PointerType const*)
   clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit(clang::Type const*)
   clang::ASTImporter::Import(clang::QualType)
   clang::QualType clang::ASTNodeImporter::importChecked<clang::QualType>(llvm::Error&, clang::QualType const&)
 
clang::ASTNodeImporter::VisitCompoundAssignOperator(clang::CompoundAssignOperator*)
mibintc marked an inline comment as done.Apr 16 2020, 7:30 AM
mibintc added a subscriber: martong.
mibintc added inline comments.
clang/lib/AST/ASTImporter.cpp
6821

@martong Thank you.

@rjmccall Can you check the patch added last night here, commit 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf
Author: Benjamin Kramer <benny.kra@googlemail.com>
Date: Thu Apr 16 11:45:02 2020 +0200

LangOptions cannot depend on ASTContext, make it not use ASTContext directly

Fixes a layering violation introduced in 2ba4e3a4598b165245c581c506a813cd4a7dce33.

I checked Benny's patch and it seems to have all the right semantics. The lit tests pass and the trailing storage is only created in the pragma case.

I also added this patch which is a companion to this.
commit 8812b0cc5cc09f350d8e89bff99f185c5e1a5d4d
Author: Melanie Blower <melanie.blower@intel.com>
Date: Thu Apr 16 08:45:26 2020 -0700

[NFC] Rename Sema.FPFeatures to CurFPFeatures and accessor to getCurFPFeatures

@rjmccall Can you check the patch added last night here, commit 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf
Author: Benjamin Kramer <benny.kra@googlemail.com>
Date: Thu Apr 16 11:45:02 2020 +0200

LangOptions cannot depend on ASTContext, make it not use ASTContext directly

Fixes a layering violation introduced in 2ba4e3a4598b165245c581c506a813cd4a7dce33.

I checked Benny's patch and it seems to have all the right semantics. The lit tests pass and the trailing storage is only created in the pragma case.

Looks good to me, too.