[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

this is implied and unneccesary

358–359

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.

458

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

460

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

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

460

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

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/Stmt.h

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;
+    unsigned FPFeatures : 7;
 
     SourceLocation OpLoc;
   };
 
   class InitListExprBitfields {
     friend class InitListExpr;
 
     unsigned : NumExprBits;
class CXXOperatorCallExprBitfields {
 
     unsigned : NumCallExprBits;
 
     /// The kind of this overloaded operator. One of the enumerator
     /// value of OverloadedOperatorKind.
     unsigned OperatorKind : 6;
 
     // Only meaningful for floating point types.
-    unsigned FPFeatures : 8;
+    unsigned FPFeatures : 7;
   };
 
   class CXXRewrittenBinaryOperatorBitfields {
     friend class ASTStmtReader;
     friend class CXXRewrittenBinaryOperator;
 
     unsigned : NumCallExprBits;
 

clang/include/clang/Basic/LangOptions.h

public:
 
   // Values of the following enumerations correspond to metadata arguments
   // specified for constrained floating-point intrinsics:
   // http://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics.
 
   /// Possible rounding modes.
   enum FPRoundingModeKind {
     /// Rounding to nearest, corresponds to "round.tonearest".
     FPR_ToNearest,
     /// Rounding toward -Inf, corresponds to "round.downward".
     FPR_Downward,
     /// Rounding toward +Inf, corresponds to "round.upward".
     FPR_Upward,
     /// Rounding toward zero, corresponds to "round.towardzero".
     FPR_TowardZero,
     /// Is determined by runtime environment, corresponds to "round.dynamic".
     FPR_Dynamic
#include "clang/Basic/LangOptions.def"
   }
 
   /// Return the OpenCL C or C++ version as a VersionTuple.
   VersionTuple getOpenCLVersionTuple() const;
 };
 
 /// Floating point control options
 class FPOptions {
 public:
-  FPOptions() : fp_contract(LangOptions::FPC_Off),
-                fenv_access(LangOptions::FEA_Off),
-                rounding(LangOptions::FPR_ToNearest),
-                exceptions(LangOptions::FPE_Ignore)
-        {}
+  FPOptions()
+      : fp_contract(LangOptions::FPC_Off), fenv_access(LangOptions::FEA_Off) {
+    setRoundingAndExceptionMode(LangOptions::FPR_ToNearest,
+                                LangOptions::FPE_Ignore);
+  }
 
   // Used for serializing.
   explicit FPOptions(unsigned I)
       : fp_contract(static_cast<LangOptions::FPContractModeKind>(I & 3)),
         fenv_access(static_cast<LangOptions::FEnvAccessModeKind>((I >> 2) & 1)),
-        rounding(static_cast<LangOptions::FPRoundingModeKind>((I >> 3) & 7)),
-        exceptions(static_cast<LangOptions::FPExceptionModeKind>((I >> 6) & 3))
-        {}
+        rounding_and_exceptions(I >> 3) {}
 
   explicit FPOptions(const LangOptions &LangOpts)
       : fp_contract(LangOpts.getDefaultFPContractMode()),
-        fenv_access(LangOptions::FEA_Off),
-        rounding(LangOptions::FPR_ToNearest),
-        exceptions(LangOptions::FPE_Ignore)
-        {}
+        fenv_access(LangOptions::FEA_Off) {
+    setRoundingAndExceptionMode(LangOptions::FPR_ToNearest,
+                                LangOptions::FPE_Ignore);
+  }
   // FIXME: Use getDefaultFEnvAccessMode() when available.
 
   bool allowFPContractWithinStatement() const {
     return fp_contract == LangOptions::FPC_On;
   }
 
   bool allowFPContractAcrossStatement() const {
     return fp_contract == LangOptions::FPC_Fast;
public:
 
   void setAllowFEnvAccess() {
     fenv_access = LangOptions::FEA_On;
   }
 
   void setDisallowFEnvAccess() { fenv_access = LangOptions::FEA_Off; }
 
   LangOptions::FPRoundingModeKind getRoundingMode() const {
-    return static_cast<LangOptions::FPRoundingModeKind>(rounding);
+     return getRoundingAndExceptionMode().first;
   }
 
   void setRoundingMode(LangOptions::FPRoundingModeKind RM) {
-    rounding = RM;
+    setRoundingAndExceptionMode(RM, getExceptionMode());
   }
 
   LangOptions::FPExceptionModeKind getExceptionMode() const {
-    return static_cast<LangOptions::FPExceptionModeKind>(exceptions);
+    return getRoundingAndExceptionMode().second;
   }
 
   void setExceptionMode(LangOptions::FPExceptionModeKind EM) {
-    exceptions = EM;
+    setRoundingAndExceptionMode(getRoundingMode(), EM);
   }
 
   bool isFPConstrained() const {
     return getRoundingMode() != LangOptions::FPR_ToNearest ||
            getExceptionMode() != LangOptions::FPE_Ignore ||
            allowFEnvAccess();
   }
 
   /// Used to serialize this.
   unsigned getInt() const {
-    return fp_contract | (fenv_access << 2) | (rounding << 3)
-        | (exceptions << 6);
+    return fp_contract | (fenv_access << 2) | (rounding_and_exceptions << 3);
   }
 
 private:
+  void setRoundingAndExceptionMode(LangOptions::FPRoundingModeKind RM,
+                                   LangOptions::FPExceptionModeKind EM) {
+    static_assert(LangOptions::FPExceptionModeKind::FPE_Strict <
+                      MaxExceptionValue,
+                  "Max exception value must be less than 3");
+    rounding_and_exceptions = RM * MaxExceptionValue + EM;
+  }
+
+  std::pair<LangOptions::FPRoundingModeKind, LangOptions::FPExceptionModeKind>
+  getRoundingAndExceptionMode() const {
+    unsigned exceptions = rounding_and_exceptions % MaxExceptionValue;
+    unsigned rounding = rounding_and_exceptions / MaxExceptionValue;
+    return {static_cast<LangOptions::FPRoundingModeKind>(rounding),
+            static_cast<LangOptions::FPExceptionModeKind>(exceptions)};
+  }
+
+
   /// Adjust BinaryOperatorBitfields::FPFeatures and
   /// CXXOperatorCallExprBitfields::FPFeatures to match the total bit-field size
   /// of these fields.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;
-  unsigned rounding : 3;
-  unsigned exceptions : 2;
+  // A packed field for encoding rounding and exceptions.
+  // FIXME: unpack this once saving one bit isn't critical here.
+  // rounding_and_exceptions = MaxExceptionValue * rounding + exceptions.
+  constexpr static unsigned MaxExceptionValue = 3;
+  unsigned rounding_and_exceptions: 4;
 };
 
 /// Describes the kind of translation unit being processed.
 enum TranslationUnitKind {
   /// The translation unit is a complete translation unit.
   TU_Complete,
 
   /// The translation unit is a prefix to a translation unit, and is