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

Thu Jun 24 11:54:42 PDT 2021

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In general, I think this looks good (I did find one minor nit that removes an include, but that can be fixed as you land). There's an open question for @jansvoboda11 and it'd be nice to hear if @joerg agrees their concerns are fully addressed, so please wait a bit before landing to give them time to respond with any concerns or feedback.

================
Comment at: clang/include/clang/Driver/Options.td:4300
 defm pack_derived : BooleanFFlag<"pack-derived">, Group<gfortran_Group>;
-defm protect_parens : BooleanFFlag<"protect-parens">, Group<gfortran_Group>;
+//defm protect_parens : BooleanFFlag<"protect-parens">, Group<gfortran_Group>;
 defm range_check : BooleanFFlag<"range-check">, Group<gfortran_Group>;
----------------
mibintc wrote:
> @jansvoboda11 This is a gfortran option, but I don't think there's a way to allow the same option spelling for gfortran and clang. Other clang options, like -short-enum, also have been pulled out of gfortran group, and the gfortran option test is an expected-fail test.
@jansvoboda11 -- do you have any ideas here? Or is the current approach fine (and we can remove this commented-out line)?

================
Comment at: clang/include/clang/Sema/Sema.h:5428
                            bool AllowRecovery = false);
+  Expr *BuildBuiltinCallExpr(SourceLocation Loc, Builtin::ID Id,
+                             MultiExprArg CallArgs);
----------------
I think it might make sense to pass in an `unsigned` rather than a `Builtin::ID` so that we can avoid including `Builtins.h` (we do this sort of dance to avoid including headers elsewhere as well). WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100118