[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 llvm-commits]

aaron.ballman added reviewers: rsmith, rjmccall.
aaron.ballman added inline comments.


================
Comment at: clang/docs/UsersManual.rst:1484
+   Where unsafe floating point optimizations are enabled, this option
+   determines whether the optimizer honors parentheses when floating-point
+   expressions are evaluated.  If unsafe floating point optimizations are
----------------
We may need to expound on what "honor parentheses" means. The summary for the patch mentions `(a + b) + c`, but does this also mean `(x + y) * z`  could be interpreted as `x + (y * z)`? What about for non-arithmetic expressions involving parentheses, like `(float1 == float2 && float1 == float3) || blah()`?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8480-8481
+def err_typecheck_expect_flt_or_vector : Error<
+  "operand of type %0 where floating, complex or "
+  "a vector of such types is required (%0 invalid)">;
 def err_cast_selector_expr : Error<
----------------



================
Comment at: clang/include/clang/Basic/TargetInfo.h:1427
 
+  /// Controls if __arithmetic_fence is supported in the targetted backend.
+  virtual bool checkArithmeticFenceSupported() const { return false; }
----------------



================
Comment at: clang/include/clang/Basic/TargetInfo.h:1162
   /// the language based on the target options where applicable.
-  virtual void adjust(LangOptions &Opts);
+  virtual void adjust(DiagnosticsEngine &Diags, LangOptions &Opts);
 
----------------
mibintc wrote:
> There's no good place to diagnose when LangOptions and TargetOptions conflict, I see "conflict" diagnostics being emitted in clang/CodeGen when creating the func or module, which seems wrong to me. On the other hand, the "adjust" function makes a good place to do the reconciliation I think. This is the change that could be pulled out in a refactoring and committed prior to this patch. What do you think? 
I think this makes sense, but should be done as a separate patch.


================
Comment at: clang/include/clang/Driver/Options.td:1757
+  LangOpts<"ProtectParens">, DefaultFalse,
+  PosFlag<SetTrue, [CC1Option], 
+          "Determines whether the optimizer honors parentheses when "
----------------
Should this option also be exposed to clang-cl (should it be a `CoreOption`)?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2839
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(*this, E);
+    auto FMF = Builder.getFastMathFlags();
+    bool isArithmeticFenceEnabled = FMF.allowReassoc();
----------------
Please spell out the type here.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2712-2714
+    case options::OPT_fprotect_parens:
+    case options::OPT_fno_protect_parens:
+      break;
----------------
Is this code needed for some reason?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6422
+  Expr *Arg = TheCall->getArg(0);
+  if (Arg->isInstantiationDependent())
+    return false;
----------------
What if there is no argument given?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6425
+
+  const QualType ArgTy = TheCall->getArg(0)->getType();
+  bool IsFloating = [&]() {
----------------



================
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();
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:6436-6437
+           << ArgTy;
+  if (checkArgCount(*this, TheCall, 1))
+    return true;
+  TheCall->setType(ArgTy);
----------------
This looks like it should move up a bit.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:387
   JustAddress = S.MaybeCreateExprWithCleanups(JustAddress);
-  return buildBuiltinCall(S, Loc, Builtin::BI__builtin_coro_resume,
-                          JustAddress);
+  return S.BuildBuiltinCallExpr(Loc, Builtin::BI__builtin_coro_resume,
+                                JustAddress).get();
----------------
I am fine ignoring this clang-format issue; I think your formatting is far more readable.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4026
+      !E->isLValue() &&
+      (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+    return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
----------------
Do you have to worry about vector types here as well?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4027
+      (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+    return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
+  }
----------------
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
}
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6535
+//  with the specified CallArgs
+ExprResult Sema::BuildBuiltinCallExpr(SourceLocation Loc, Builtin::ID Id,
+                                      MultiExprArg CallArgs) {
----------------
Because this cannot fail, is there a benefit to returning an `ExprResult` rather than an `Expr *` as before? Then you can remove a bunch of `get()` calls elsewhere that look suspicious because they're not checking for a failure.


================
Comment at: clang/test/Sema/arithmetic-fence-builtin.c:45
+#endif
+//PPC: error: option '-fprotect-parens' cannot be specified on this target
----------------
You should add a test that we diagnose things like:
```
__arithmetic_fence();
__arithmetic_fence(1, 2);
```
I think it would also be useful to add an AST test (to `clang\test\AST`) that shows the call expression AST nodes being inserted for paren expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118