[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 6 inline comments as done.
mibintc added a subscriber: yaxunl.
mibintc added a comment.

@rjmccall I added some inline questions and comments for you.  Thanks a lot for your review.



================
Comment at: clang/include/clang/AST/ExprCXX.h:172
+  FPOptionsOverride getFPFeatures() const {
+    return FPOptionsOverride(CXXOperatorCallExprBits.FPFeatures);
   }
----------------
It seems that this field is  basically not used, if I eliminate all these member functions then the AST reader-writer fails but the information isn't being used in Sema or Codegen. I guess there's a bug somewhere if this information is actually needed when creating Float comparison codegen.  Also,, from reading the comments in this file it sems that it is needed only when "rewrite == operator into OperatorCall as required by CXX20", the comments also say that the OperatorCall rewrite could equally have occurred as a rewrite into a BinaryOperator.  If we choose to rewrite the == as BinaryOperator then it wouldn't be necessary to use TrailingStorage to hold FPOptionsOverride.  The previously proposed revision didn't  adding TrailingStorage on CXXOperatorCall.  When I was working on a different patch I did study how to add TrailingStorage to a Call and I didn't know how I could accomplish that. 


================
Comment at: clang/include/clang/AST/Stmt.h:620
+    //unsigned FPFeatures : 21;
+    unsigned FPFeatures : 32;
   };
----------------
This is a temporary change, I know you don't want me to change the assert to allow a wider size than 8. So if this information is really needed (as I mentioned above it's not being used in Sema or Codegen) then alternatives are
1. rewrite the == comparison using BinaryOperator
2. put the FPFeatures Override into TrailingStorage (i think i will need implementation guidance if this is the path to go)


================
Comment at: clang/include/clang/AST/Stmt.h:1121
   Stmt(StmtClass SC) {
-    static_assert(sizeof(*this) <= 8,
+    static_assert(sizeof(*this) <= 16,
                   "changing bitfields changed sizeof(Stmt)");
----------------
this is a temporary change


================
Comment at: clang/include/clang/Basic/LangOptions.def:192
 COMPATIBLE_LANGOPT(Deprecated        , 1, 0, "__DEPRECATED predefined macro")
-COMPATIBLE_LANGOPT(FastMath          , 1, 0, "fast FP math optimizations, and __FAST_MATH__ predefined macro")
-COMPATIBLE_LANGOPT(FiniteMathOnly    , 1, 0, "__FINITE_MATH_ONLY__ predefined macro")
-COMPATIBLE_LANGOPT(UnsafeFPMath      , 1, 0, "Unsafe Floating Point Math")
-COMPATIBLE_LANGOPT(AllowFPReassoc    , 1, 0, "Permit Floating Point reassociation")
-COMPATIBLE_LANGOPT(NoHonorNaNs       , 1, 0, "Permit Floating Point optimization without regard to NaN")
-COMPATIBLE_LANGOPT(NoHonorInfs       , 1, 0, "Permit Floating Point optimization without regard to infinities")
-COMPATIBLE_LANGOPT(NoSignedZero      , 1, 0, "Permit Floating Point optimization without regard to signed zeros")
-COMPATIBLE_LANGOPT(AllowRecip        , 1, 0, "Permit Floating Point reciprocal")
-COMPATIBLE_LANGOPT(ApproxFunc        , 1, 0, "Permit Floating Point approximation")
+BENIGN_LANGOPT(FastMath          , 1, 0, "fast FP math optimizations, and __FAST_MATH__ predefined macro")
+BENIGN_LANGOPT(FiniteMathOnly    , 1, 0, "__FINITE_MATH_ONLY__ predefined macro")
----------------
Here's another problem, I added the test case from bug 46166 @yaxunl but that test case reports incompatibility because the pch is compiled with different settings than where it is used. I thought maybe based on the name, BENIGN_LANGOPT would  allow different option values to not get the pch complaint but that didn't work, clang still complained about the different option values.  Is there some existing way to get around the option complaint or do I need to invent a new field in the LANGOPT e.g. a boolean that says "Allow opton values to differ between pch-create and pch-use".  I haven't deeply studied this to see if I'm missing something. I wanted to send this out to get comments on my other questions 


================
Comment at: clang/include/clang/Basic/LangOptions.h:455
+  llvm::errs() << "\n RoundingMode " <<
+        static_cast<unsigned>(getRoundingMode());
+  llvm::errs() << "\n FPExceptionMode " << getFPExceptionMode();
----------------
Using #define OPTION trick would be preferrable except there is a compilation failure because something funny about RoundingMode--need the cast. doubtless there is fix for that


================
Comment at: clang/lib/Parse/ParsePragma.cpp:657
 
-  Actions.ActOnPragmaFPContract(FPC);
-  ConsumeAnnotationToken();
+  SourceLocation PragmaLoc = ConsumeAnnotationToken();
+  Actions.ActOnPragmaFPContract(PragmaLoc, FPC);
----------------
Note: This patch is putting all the pragma's that modify floating point state into the FpPragmaStack, using the "Set" call to set the top value of the FP pragma stack.  Of course the pragma's that affect the "fp contract" and "reassociate" etc. don't support stacking settings e.g. with PUSH and POP.


================
Comment at: clang/test/SemaOpenCL/fp-options.cl:1
+// RUN: %clang_cc1 %s -finclude-default-header -triple spir-unknown-unknown -emit-pch -o %t.pch
+// RUN: %clang_cc1 %s -cl-no-signed-zeros -triple spir-unknown-unknown -include-pch %t.pch -fsyntax-only -verify
----------------
Here's the new test case from @yaxunl  that I mentioned above. This test is failing. Besides that, check-all is passing all the  lit tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81869