[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[John McCall via Phabricator via cfe-commits]

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