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

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added a comment.

I added some inline replies to John.  I'm not certain I have everything yet exactly the way he wants it.



================
Comment at: clang/include/clang/AST/Expr.h:2573
 
+  FPOptions FPFeatures;
+
----------------
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.


================
Comment at: clang/include/clang/AST/Expr.h:3417
+    : public Expr,
+      private llvm::TrailingObjects<BinaryOperator, unsigned, QualType> {
   enum { LHS, RHS, END_EXPR };
----------------
rjmccall wrote:
> Why does this use `unsigned` for the trailing storage of the `FPOptions`?
I will change it to FPOptions


================
Comment at: clang/include/clang/AST/Expr.h:3420
   Stmt *SubExprs[END_EXPR];
+  bool hasFPFeatures;
+  bool isCompoundAssign;
----------------
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. 


================
Comment at: clang/include/clang/AST/Expr.h:3421
+  bool hasFPFeatures;
+  bool isCompoundAssign;
+
----------------
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


================
Comment at: clang/include/clang/AST/Expr.h:3465
+    SubExprs[RHS] = rhs;
+    hasFPFeatures = true;
+    isCompoundAssign = 1;
----------------
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. 


================
Comment at: clang/include/clang/AST/Expr.h:3471
+    assert(isCompoundAssignmentOp() &&
+           "Expected CompoundAssignOperator for compound assignments");
+    setComputationLHSType(CompLHSType);
----------------
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?


================
Comment at: clang/include/clang/AST/Expr.h:6029
   friend class ASTStmtWriter;
 };
 
----------------
rjmccall wrote:
> Why is this in this patch?
I'm not sure what this is referring to?


================
Comment at: clang/include/clang/AST/StmtVisitor.h:92
+      case BO_XorAssign:
+        DISPATCH(BinXorAssign, BinaryOperator);
       case BO_Comma:     DISPATCH(BinComma,     BinaryOperator);
----------------
rjmccall wrote:
> Let's stick with the existing formatting for consistency.  It's just vertically aligning the DISPATCH and class-name tokens to emphasize the common pattern.
You had mentioned in your previous review about having things lined up properly here, and I don't disagree--this comment is just to let you know that I had applied the clang-format patch that was supplied by the BuildBots in the pre-commit checks. The buildbot pre-commit check wants them on different lines. I've switched the formatting to what you request.


================
Comment at: clang/include/clang/Basic/LangOptions.h:379
+
+  /// Return the default value of FPOptions that's used when trailing
+  /// storage isn't required.
----------------
I added these at your suggestion but not presently making use of them


================
Comment at: clang/lib/AST/Expr.cpp:4285
+                                       FPOptions FPFeatures) {
+  bool hasFPFeatures = true;
+  unsigned SizeOfTrailingObjects =
----------------
rjmccall wrote:
> These places should use that same unified predicate.
I'm not sure if I captured what you want here. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76384/new/

https://reviews.llvm.org/D76384