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