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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1757
+  LangOpts<"ProtectParens">, DefaultFalse,
+  PosFlag<SetTrue, [CC1Option], 
+          "Determines whether the optimizer honors parentheses when "
----------------
mibintc wrote:
> 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? 
I think this would be useful for clang-cl users, so I'd recommend adding it to `CoreOption` unless there's a reason not to.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9329-9331
+  case Builtin::BI__arithmetic_fence:
+    return false;
+
----------------
Under what circumstances could we get here? (Given that this is within `PointerExprEvaluator`, I would have assumed this would be unreachable code.)


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2841
+    llvm::FastMathFlags FMF = Builder.getFastMathFlags();
+    bool isArithmeticFenceEnabled = FMF.allowReassoc() &&
+	   getContext().getTargetInfo().checkArithmeticFenceSupported();
----------------
May as well fix this clang-format issue.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6568
+Expr *Sema::BuildBuiltinCallExpr(SourceLocation Loc, Builtin::ID Id,
+                                      MultiExprArg CallArgs) {
+  StringRef Name = Context.BuiltinInfo.getName(Id);
----------------
Might as well hit this clang-format warning too.


================
Comment at: clang/test/AST/arithmetic-fence-builtin.c:34
+
+  v = (a + b);
+
----------------
Does the `(a + b)` still have an AST node for the `ParenExpr`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118