[AST] Unpack FPFeatures bits to BinaryOperator, NFC.

[hokein]

Summary

The stmt bit-fields is full (max 64 bits) for BinaryOperator now, adding
a new bit field (error) causes an 'static_assert(sizeof(*this) <=8)'
violation in Stmt constructor.

This patch unpacks the FPFeautres, make available bitfields for error
bit (https://reviews.llvm.org/D65591).

[mibintc]

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

[sammccall]

As discussed offline... this undoes a desirable optimization from D54526.

A couple of possible ways to avoid this:

• FPOptions integer encoding can fit into 7 bits: combine integer behavior + exception behavior into 3 x 5 = 15 states that fits in 4 bits.
• IsOMPStructuredBlock bit is basically unused (just for AST dump and an unused ASTMatcher), that bit could be dropped

Former is probably the eaisest.

[hokein]

reduce FPOption from 8 bits to 7 bits, based on the offline discussion.

[hokein]

update a missing CXXOperatorCallExprBitfields, and fix typos.

[sammccall]

In D75443#1916761, @mibintc wrote:

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

Great! Moving FPOptions to trailing storage (which I guess allows it to only be present for floating point ops?) is much better than making it a member, and frees up a lot of bits.

To unblock adding the extra "error" bit that we need (which really does apply to all Exprs), we could do any of:

1. wait for D72841 to land, including the trailingstorage
2. move the trailing-storage parts out of that patch, and do it first as a smaller separate change
3. find some other way to save the bit for now (e.g. by encoding rounding + exceptions together), and undo that once saving 1 bit doesn't matter

Any objections if we do 3 and take care of rolling it back later?

clang/include/clang/Basic/LangOptions.h

200 ↗

(On Diff #249607)

this is implied and unneccesary

361 ↗

(On Diff #249607)

Making use of the encoding of packed in lots of places is fragile and hard to maintain.

I'd suggest the private primitives:
setRoundingAndExceptionMode(FPRoundingModeKind, FPExceptionModeKind)
getRoundingAndExceptionMode() -> pair<FPRoundingModeKind, FPExceptionModeKind>
and implementing everything in terms of them.

445 ↗

(On Diff #249607)

FIXME: unpack this once saving one bit isn't critical here.

447 ↗

(On Diff #249607)

rounding_and_exceptions?

[hokein]

address comments.

[hokein]

In D75443#1916829, @sammccall wrote:

In D75443#1916761, @mibintc wrote:

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

Great! Moving FPOptions to trailing storage (which I guess allows it to only be present for floating point ops?) is much better than making it a member, and frees up a lot of bits.

To unblock adding the extra "error" bit that we need (which really does apply to all Exprs), we could do any of:

1. wait for D72841 to land, including the trailingstorage
2. move the trailing-storage parts out of that patch, and do it first as a smaller separate change
3. find some other way to save the bit for now (e.g. by encoding rounding + exceptions together), and undo that once saving 1 bit doesn't matter

Any objections if we do 3 and take care of rolling it back later?

I think waiting for 1 would probably take a long time since that is a huge patch. I'd prefer 3 to unblock the "error" bit stuff, but I'm also happy with 2. what's your thought @mibintc?

[mibintc]

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

[sammccall]

this looks reasonable to me, would like to give @mibintc and also @sepavloff a chance to chime in.

clang/include/clang/Basic/LangOptions.h

437 ↗

(On Diff #249613)

This assertion seems weird, and it doesn't test much - new enum values are typically added at the end.

460 ↗

(On Diff #249613)

nit: NumExceptionModes ?
(It's not the max value, it's max + 1)

[sammccall]

In D75443#1916909, @mibintc wrote:

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

(Sorry our comments keep racing)

If you don't mind doing that, it'd be great - phabricator is the best way.
I'm happy to do my best reviewing, but haven't made a similar change myself, so getting rjmccall's eyes on it would definitely get you a better check.

[hokein]

In D75443#1916909, @mibintc wrote:

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

that sounds good to me, thanks!

[sepavloff]

clang/include/clang/Basic/LangOptions.h

461 ↗

(On Diff #249613)

There are 5 rounding directions defined by IEEE-754, which we should eventually support. Some targets supports additional rounding modes, which we might want to keep in FP environment. Existing implementation uses 3 exception behavior modes and we could want to extend them. For example it makes sense to introduce a special mode where Inexact exception is ignored but others are maintained. Four bits are too small space for FP environment.

[mibintc]

I ran into an obstacle trying to add Trailing storage onto binary operator. it will probably take me longer.

[hokein]

In D75443#1917351, @mibintc wrote:

I ran into an obstacle trying to add Trailing storage onto binary operator. it will probably take me longer.

sure, no worries, and thanks for your help. It is not super urgent for us at the moment (we have other patches under review).

[hokein]

woo, looks like the IsOMPStructuredBlock bit in Stmt was removed in https://reviews.llvm.org/rGd5edcb90643104d6911da5c0ff44c4f33fff992f, we can use the bit for error, so this patch is not needed anymore.

clang/include/clang/AST/Expr.h

 /// 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 {
   enum { LHS, RHS, END_EXPR };
   Stmt *SubExprs[END_EXPR];
-
+  /// This is only meaningful for operations on floating point
+  /// types and 0 otherwise.
+  FPOptions FPFeatures;
 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())) {
+            rhs->containsUnexpandedParameterPack())),
+            FPFeatures(FPFeatures) {
     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.
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) {
-    BinaryOperatorBits.FPFeatures = F.getInt();
+    FPFeatures = F;
   }
 
   FPOptions getFPFeatures() const {
-    return FPOptions(BinaryOperatorBits.FPFeatures);
+    return FPFeatures;
   }
 
   // Get the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.
   bool isFPContractableWithinStatement() const {
     return getFPFeatures().allowFPContractWithinStatement();
   }
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floating point types.
   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())) {
+            rhs->containsUnexpandedParameterPack())),
+            FPFeatures(FPFeatures) {
     BinaryOperatorBits.Opc = opc;
-    BinaryOperatorBits.FPFeatures = FPFeatures.getInt();
     BinaryOperatorBits.OpLoc = opLoc;
     SubExprs[LHS] = lhs;
     SubExprs[RHS] = rhs;
   }
 
   BinaryOperator(StmtClass SC, EmptyShell Empty) : Expr(SC, Empty) {
     BinaryOperatorBits.Opc = BO_MulAssign;
   }

clang/include/clang/AST/Stmt.h

protected:
 
   class BinaryOperatorBitfields {
     friend class BinaryOperator;
 
     unsigned : NumExprBits;
 
     unsigned Opc : 6;
 
-    /// This is only meaningful for operations on floating point
-    /// types and 0 otherwise.
-    unsigned FPFeatures : 8;
-
     SourceLocation OpLoc;
   };
 
   class InitListExprBitfields {
     friend class InitListExpr;
 
     unsigned : NumExprBits;