rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.
Ok, with the break fix, this looks goof to me. Thanks!
================
Comment at: lib/Driver/ToolChains/Clang.cpp:2452
+ if (!HonorInfs && !HonorNans && !MathErrno && AssociativeMath &&
+ ReciprocalMath && !SignedZeros && !TrappingMath && FpContract == "fast")
+ CmdArgs.push_back("-ffast-math");
----------------
john.brawn wrote:
> 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.
Right, makes sense.
================
Comment at: lib/Driver/ToolChains/Clang.cpp:2347
+ // Validate and pass through -fp-contract option.
+ case options::OPT_ffp_contract: {
+ StringRef Val = A->getValue();
----------------
john.brawn wrote:
> 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.
The former is done by overall design, the latter is different to most other
options because of what "on" means and how it's *not* implemented, meaning
it'll be effectively "off". Otherwise, I'd completely agree with you.
But that's not a strong point, for me. I'm happy to not have that now and
involve this into a more general "what does on mean" later on.
Repository:
rL LLVM
https://reviews.llvm.org/D30582
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
