rjmccall added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:2573
+ FPOptions FPFeatures;
+
----------------
mibintc wrote:
> rjmccall wrote:
> > 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.
> 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.
Sure, check out CallExpr's sizeOfTrailingObjects and offsetToTrailingObjects.
================
Comment at: clang/include/clang/AST/Expr.h:3420
Stmt *SubExprs[END_EXPR];
+ bool hasFPFeatures;
+ bool isCompoundAssign;
----------------
mibintc wrote:
> 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.
It's pretty common to make ASTStmtReader and ASTStmtWriter friends of the
bitfields struct.
================
Comment at: clang/include/clang/AST/Expr.h:3421
+ bool hasFPFeatures;
+ bool isCompoundAssign;
+
----------------
mibintc wrote:
> rjmccall wrote:
> > Can these bits go in the bitfields?
> 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
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.
================
Comment at: clang/include/clang/AST/Expr.h:3465
+ SubExprs[RHS] = rhs;
+ hasFPFeatures = true;
+ isCompoundAssign = 1;
----------------
mibintc wrote:
> rjmccall wrote:
> > mibintc wrote:
> > > rjmccall wrote:
> > > > 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() { ... }
> > > > ```
> > > 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.
> > 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()`?
> 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.
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.
================
Comment at: clang/include/clang/AST/Expr.h:3471
+ assert(isCompoundAssignmentOp() &&
+ "Expected CompoundAssignOperator for compound assignments");
+ setComputationLHSType(CompLHSType);
----------------
mibintc wrote:
> rjmccall wrote:
> > 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.
> 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?
`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(...);
```
================
Comment at: clang/include/clang/AST/Expr.h:6029
friend class ASTStmtWriter;
};
----------------
mibintc wrote:
> rjmccall wrote:
> > Why is this in this patch?
> I'm not sure what this is referring to?
I was probably looking at the wrong diff, sorry. The "new changes only"
display can get confused when patches are rebased.
================
Comment at: clang/include/clang/AST/Expr.h:3669
+
+ bool IsCompoundAssign() const {
+ auto opc = static_cast<Opcode>(BinaryOperatorBits.Opc);
----------------
We generally lower-case method names, so these would be `isCompoundAssign()`
and `hasFPFeatures()` (maybe `hasFPFeatureOverrides()`?).
================
Comment at: clang/include/clang/Basic/LangOptions.h:379
+
+ /// Return the default value of FPOptions that's used when trailing
+ /// storage isn't required.
----------------
mibintc wrote:
> I added these at your suggestion but not presently making use of them
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.
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
