sepavloff added inline comments.
================ Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + ---------------- rjmccall wrote: > sepavloff wrote: > > erichkeane wrote: > > > rjmccall wrote: > > > > erichkeane wrote: > > > > > rjmccall wrote: > > > > > > rjmccall wrote: > > > > > > > erichkeane wrote: > > > > > > > > rjmccall wrote: > > > > > > > > > The problem with having both functions that take > > > > > > > > > `ASTContext`s 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 > > > > > > > > > `ASTContext`s.. > > > > > > > > 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. > > > > > > > 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. > > > > > > > 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. > > > > > 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". > > > > 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`. > > > That sounds like a good plan to me. Thanks for entertaining my > > > conversation/questions. > > > 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. > 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. > 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
