[AST] Pack BinaryOperator

[riccibruno]

Summary

Use the newly available space in the bit-fields of Stmt.
This saves 8 bytes per BinaryOperator.

[dblaikie]

Looks good - thanks! just a few things that could be cleaned up.

include/clang/AST/Expr.h

3220–3222

Lost LLVM_READONLY somewhere along the way? Should it be added back in?

3256–3261

I probably wouldn't add a new public member function here - I take it you added that to avoid calling getOpcode() twice? (either for performance or textual/source tersity?)

Calling it twice probably is OK for both performance and readability - but if you prefer otherwise, I guess you could do it with a temporary:

bool isPetrMemOp() const {
  auto Opc = getOpcode();
  return Opc == ... ;
}

3369–3375

Could do these two (& some of the other "refactor to use a common accessor, because the accessor is getting a bit more interesting") changes as NFC cleanup before this - but no big deal, just mention it in case in other/future changes that makes sense for you.

[dblaikie]

include/clang/AST/Stmt.h

572

Oh, just a thought - wonder if we could/should have an assert that this anonymous union doesn't grow beyond a certain intended size? (can we have such an assert? I'm not sure how to test the size of an anonymous union - I guess we could static_assert the size of the whole Stmt class, though that'd be a bit brittle)

[riccibruno]

Added some inline comments.

include/clang/AST/Expr.h

3220–3222

On purpose. LLVM_READONLY on a simple getter seems pointless,
since the compiler is just going to inline it.

3259

I added it for consistency with the other similar function below.
isPtrMemOp is the only one without the pattern of having
a static member function and a member function calling it.
I would not expect this to matter for performance since the compiler
is just going to see right through the getOpcode call.

include/clang/AST/Stmt.h

572

Already done in line 657 below. And yes it is a valid concern !
When I did the same thing for the types hierarchy I spotted
that the bit-field class FunctionTypeBitfields was taking more
than 32 bits accidentally. (fixed since) Oops.

[riccibruno]

Marked the inline comments as done since I believe I
answered each of them. If not I can fix it in a subsequent commit.

include/clang/AST/Expr.h

 /// used to store an expression "x + y" depends on the subexpressions
 /// for x and y. If neither x or y is type-dependent, and the "+"
 /// operator resolves to a built-in operation, BinaryOperator will be
 /// used to express the computation (x and y may still be
 /// value-dependent). If either x or y is type-dependent, or if the
 /// "+" resolves to an overloaded operator, CXXOperatorCallExpr will
 /// be used to express the computation.
 class BinaryOperator : public Expr {
-public:
-  typedef BinaryOperatorKind Opcode;
-
-private:
-  unsigned Opc : 6;
-
-  // This is only meaningful for operations on floating point types and 0
-  // otherwise.
-  unsigned FPFeatures : 3;
-  SourceLocation OpLoc;
-
   enum { LHS, RHS, END_EXPR };
-  Stmt* SubExprs[END_EXPR];
+  Stmt *SubExprs[END_EXPR];
+
 public:
+  typedef BinaryOperatorKind Opcode;
 
   BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
                  ExprValueKind VK, ExprObjectKind OK,
                  SourceLocation opLoc, FPOptions FPFeatures)
     : Expr(BinaryOperatorClass, ResTy, VK, OK,
            lhs->isTypeDependent() || rhs->isTypeDependent(),
            lhs->isValueDependent() || rhs->isValueDependent(),
            (lhs->isInstantiationDependent() ||
             rhs->isInstantiationDependent()),
            (lhs->containsUnexpandedParameterPack() ||
-            rhs->containsUnexpandedParameterPack())),
-      Opc(opc), FPFeatures(FPFeatures.getInt()), OpLoc(opLoc) {
+            rhs->containsUnexpandedParameterPack())) {
+    BinaryOperatorBits.Opc = opc;
+    BinaryOperatorBits.FPFeatures = FPFeatures.getInt();
+    BinaryOperatorBits.OpLoc = opLoc;
     SubExprs[LHS] = lhs;
     SubExprs[RHS] = rhs;
     assert(!isCompoundAssignmentOp() &&
            "Use CompoundAssignOperator for compound assignments");
   }
 
   /// Construct an empty binary operator.
-  explicit BinaryOperator(EmptyShell Empty)
-    : Expr(BinaryOperatorClass, Empty), Opc(BO_Comma) { }
+  explicit BinaryOperator(EmptyShell Empty) : Expr(BinaryOperatorClass, Empty) {
+    BinaryOperatorBits.Opc = BO_Comma;
+  }
 
-  SourceLocation getExprLoc() const LLVM_READONLY { return OpLoc; }
-  SourceLocation getOperatorLoc() const { return OpLoc; }
-  void setOperatorLoc(SourceLocation L) { OpLoc = L; }
+  SourceLocation getExprLoc() const { return getOperatorLoc(); }
+  SourceLocation getOperatorLoc() const { return BinaryOperatorBits.OpLoc; }
+  void setOperatorLoc(SourceLocation L) { BinaryOperatorBits.OpLoc = L; }
 
-  Opcode getOpcode() const { return static_cast<Opcode>(Opc); }
-  void setOpcode(Opcode O) { Opc = O; }
+  Opcode getOpcode() const {
+    return static_cast<Opcode>(BinaryOperatorBits.Opc);
+  }
+  void setOpcode(Opcode Opc) { BinaryOperatorBits.Opc = Opc; }
 
   Expr *getLHS() const { return cast<Expr>(SubExprs[LHS]); }
   void setLHS(Expr *E) { SubExprs[LHS] = E; }
   Expr *getRHS() const { return cast<Expr>(SubExprs[RHS]); }
   void setRHS(Expr *E) { SubExprs[RHS] = E; }
 
   SourceLocation getBeginLoc() const LLVM_READONLY {
     return getLHS()->getBeginLoc();
public:
   /// overloaded operator.
   static Opcode getOverloadedOpcode(OverloadedOperatorKind OO);
 
   /// Retrieve the overloaded operator kind that corresponds to
   /// the given binary opcode.
   static OverloadedOperatorKind getOverloadedOperator(Opcode Opc);
 
   /// predicates to categorize the respective opcodes.
-  bool isPtrMemOp() const { return Opc == BO_PtrMemD || Opc == BO_PtrMemI; }
+  static bool isPtrMemOp(Opcode Opc) {
+    return Opc == BO_PtrMemD || Opc == BO_PtrMemI;
+  }
+  bool isPtrMemOp() const { return isPtrMemOp(getOpcode()); }
+
   static bool isMultiplicativeOp(Opcode Opc) {
     return Opc >= BO_Mul && Opc <= BO_Rem;
   }
   bool isMultiplicativeOp() const { return isMultiplicativeOp(getOpcode()); }
   static bool isAdditiveOp(Opcode Opc) { return Opc == BO_Add || Opc==BO_Sub; }
   bool isAdditiveOp() const { return isAdditiveOp(getOpcode()); }
   static bool isShiftOp(Opcode Opc) { return Opc == BO_Shl || Opc == BO_Shr; }
   bool isShiftOp() const { return isShiftOp(getOpcode()); }
 
child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+END_EXPR);
   }
   const_child_range children() const {
     return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
   }
 
   // Set the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.
-  void setFPFeatures(FPOptions F) { FPFeatures = F.getInt(); }
+  void setFPFeatures(FPOptions F) {
+    BinaryOperatorBits.FPFeatures = F.getInt();
+  }
 
-  FPOptions getFPFeatures() const { return FPOptions(FPFeatures); }
+  FPOptions getFPFeatures() const {
+    return FPOptions(BinaryOperatorBits.FPFeatures);
+  }
 
   // Get the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.
   bool isFPContractableWithinStatement() const {
-    return FPOptions(FPFeatures).allowFPContractWithinStatement();
+    return getFPFeatures().allowFPContractWithinStatement();
   }
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floating point types.
-  bool isFEnvAccessOn() const {
-    return FPOptions(FPFeatures).allowFEnvAccess();
-  }
+  bool isFEnvAccessOn() const { return getFPFeatures().allowFEnvAccess(); }
 
 protected:
   BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
                  ExprValueKind VK, ExprObjectKind OK,
                  SourceLocation opLoc, FPOptions FPFeatures, bool dead2)
     : Expr(CompoundAssignOperatorClass, ResTy, VK, OK,
            lhs->isTypeDependent() || rhs->isTypeDependent(),
            lhs->isValueDependent() || rhs->isValueDependent(),
            (lhs->isInstantiationDependent() ||
             rhs->isInstantiationDependent()),
            (lhs->containsUnexpandedParameterPack() ||
-            rhs->containsUnexpandedParameterPack())),
-      Opc(opc), FPFeatures(FPFeatures.getInt()), OpLoc(opLoc) {
+            rhs->containsUnexpandedParameterPack())) {
+    BinaryOperatorBits.Opc = opc;
+    BinaryOperatorBits.FPFeatures = FPFeatures.getInt();
+    BinaryOperatorBits.OpLoc = opLoc;
     SubExprs[LHS] = lhs;
     SubExprs[RHS] = rhs;
   }
 
-  BinaryOperator(StmtClass SC, EmptyShell Empty)
-    : Expr(SC, Empty), Opc(BO_MulAssign) { }
+  BinaryOperator(StmtClass SC, EmptyShell Empty) : Expr(SC, Empty) {
+    BinaryOperatorBits.Opc = BO_MulAssign;
+  }
 };
 
 /// CompoundAssignOperator - For compound assignments (e.g. +=), we keep
 /// track of the type the operation is performed in.  Due to the semantics of
 /// these operators, the operands are promoted, the arithmetic performed, an
 /// implicit conversion back to the result type done, then the assignment takes
 /// place.  This captures the intermediate type which the computation is done
 /// in.

include/clang/AST/Stmt.h

class CastExprBitfields {
 
     unsigned : NumExprBits;
 
     unsigned Kind : 6;
     unsigned PartOfExplicitCast : 1; // Only set for ImplicitCastExpr.
     unsigned BasePathIsEmpty : 1;
   };
 
+  class BinaryOperatorBitfields {
+    friend class BinaryOperator;
+
+    unsigned : NumExprBits;
+
+    unsigned Opc : 6;
+    unsigned FPFeatures : 3;
+
+    SourceLocation OpLoc;
+  };
+
   class InitListExprBitfields {
     friend class InitListExpr;
 
     unsigned : NumExprBits;
 
     /// Whether this initializer list originally had a GNU array-range
     /// designator in it. This is a temporary marker used by CodeGen.
     unsigned HadArrayRangeDesignator : 1;
union {
     DeclRefExprBitfields DeclRefExprBits;
     FloatingLiteralBitfields FloatingLiteralBits;
     CharacterLiteralBitfields CharacterLiteralBits;
     UnaryOperatorBitfields UnaryOperatorBits;
     UnaryExprOrTypeTraitExprBitfields UnaryExprOrTypeTraitExprBits;
     CallExprBitfields CallExprBits;
     MemberExprBitfields MemberExprBits;
     CastExprBitfields CastExprBits;
+    BinaryOperatorBitfields BinaryOperatorBits;
     InitListExprBitfields InitListExprBits;
     PseudoObjectExprBitfields PseudoObjectExprBits;
 
     // C++ Expressions
     TypeTraitExprBitfields TypeTraitExprBits;
     ExprWithCleanupsBitfields ExprWithCleanupsBits;
 
     // C++ Coroutines TS expressions