[PATCH] D100118: [clang] Add support for new builtin __arithmetic_fence to control floating point optimization, and new clang option fprotect-parens

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 9 inline comments as done.
mibintc added a comment.

A couple inline replies to go along with the updated patch



================
Comment at: clang/include/clang/Driver/Options.td:1757
+  LangOpts<"ProtectParens">, DefaultFalse,
+  PosFlag<SetTrue, [CC1Option], 
+          "Determines whether the optimizer honors parentheses when "
----------------
aaron.ballman wrote:
> Should this option also be exposed to clang-cl (should it be a `CoreOption`)?
I don't know the answer to this, do you have a recommendation? 


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6427-6430
+    if (const VectorType *VT = dyn_cast<VectorType>(ArgTy.getCanonicalType()))
+      return VT->getElementType().getTypePtr()->isFloatingType();
+    if (const ComplexType *CT = dyn_cast<ComplexType>(ArgTy.getCanonicalType()))
+      return CT->getElementType().getTypePtr()->isFloatingType();
----------------
aaron.ballman wrote:
> 
I found there was a boolean function, so i got rid of the lambda. 


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:294
 
-static Expr *buildBuiltinCall(Sema &S, SourceLocation Loc, Builtin::ID Id,
-                              MultiExprArg CallArgs) {
----------------
I moved this function to public because another file is also using the same logic


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4026
+      !E->isLValue() &&
+      (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+    return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
----------------
aaron.ballman wrote:
> Do you have to worry about vector types here as well?
Oh yes, i didn't get this one. 


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4027
+      (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+    return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
+  }
----------------
aaron.ballman wrote:
> One worry I have about this is that the AST will be a bit mysterious for people writing AST matchers. The source code will have parens in it, but the AST will have a `ParenExpr` node or not only depending on the language options. This may also cause problems for other parts of the code that are checking for properly-parenthesized expressions (like && and || within the same expression), so we should probably have a test case like:
> ```
> bool func(float f1, float f2, float f3) {
>   return (f1 == f2 && f1 == f3) || f2 == f3; // Should not warn here
> }
> ```
I added an AST test case 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118