[PATCH] D30582: [Driver] Restructure handling of -ffast-math and similar options

John Brawn via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 15 05:45:03 PDT 2017

john.brawn added inline comments.

Comment at: lib/Driver/ToolChains/Clang.cpp:2452
+  if (!HonorInfs && !HonorNans && !MathErrno && AssociativeMath &&
+      ReciprocalMath && !SignedZeros && !TrappingMath && FpContract == "fast")
+    CmdArgs.push_back("-ffast-math");
rengolin wrote:
> rengolin wrote:
> > This is technically correct, but users will be confused if they choose `-ffast-math -ffp-contract=on` and not see `-ffast-math` coming out on the other side.
> > 
> > Also, `fp-contract=on` doesn't preclude `-ffast-math` for the languages that support it, so I wouldn't add `FpContract` to this list at all.
> I've been thinking a bit more about this, and I started wondering, why do we even need to pass `-ffast-math` down?
> If all the others are already being passed, shouldn't this flag be redundant?
> Finally, we could possibly add instead `&& FpContract != "off"`. Would that be better?
As the comment above says, -ffast-math enables the __FAST_MATH__ macro.

As to FpContract, going back and checking gcc setting -ffp-contract has no effect on whether __FAST_MATH__ is defined so I think just not checking FpContract here is correct.

Comment at: lib/Driver/ToolChains/Clang.cpp:2347
+    // Validate and pass through -fp-contract option.
+    case options::OPT_ffp_contract: {
+      StringRef Val = A->getValue();
rengolin wrote:
> Also, when `-ffast-math` is selected, and `-ffp-contract=on`, we should actually change it to `fast`, no?
Do you mean "clang -ffp-contract=on -ffast-math should set fp-contract to fast" or "clang -ffast-math -ffp-contract=on should set fp-contract to fast"? The first is already done by the -ffast-math handling below, and I think the second is a bad idea because it violates the principle that later command-line options have priority over earlier ones.

Comment at: lib/Driver/ToolChains/Clang.cpp:2356
+    }
+    case options::OPT_ffinite_math_only:
rengolin wrote:
> Missing break?
Whoops, will fix.



More information about the cfe-commits mailing list