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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
