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