rjmccall added inline comments.
================ Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + ---------------- sepavloff wrote: > 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. 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. 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
