mibintc marked 4 inline comments as done.
mibintc added a comment.
inline replies to Michele, will upload a new patch shortly
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2408
+ case options::OPT_frounding_math:
+ // The default setting for frounding-math is True and ffast-math
+ // sets fno-rounding-math, but we only want to use constrained
----------------
michele.scandale wrote:
> Isn't the default `-fno-rounding-math`?
Yes the default is no rounding math, I'll remove the comment. Thank you.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2416
+ RoundingFPMath = false;
+ RoundingMathPresent = false;
+ break;
----------------
michele.scandale wrote:
> Shouldn't this be set to `true` similarly to what you do for
> `TrappingMathPresent ` to track whether there is an explicit option related
> to rounding math?
There's a switch statement above this that interprets the command line option
-fp-model=strict as though frounding had appeared on the command line by
assigning a new value to optID so that's why there is a discrepancy. Also I'm
using the *Present boolean variables to preserve the output from Driver so that
pre-existing driver test cases don't need to be changed.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2574
+ // FP Exception Behavior is also set to strict
+ assert(FPExceptionBehavior.equals("strict"));
+ CmdArgs.push_back("-ftrapping-math");
----------------
michele.scandale wrote:
> Running `clang -### -ftrapping-math -ffp-exception-behavior=ignore` lead to
> this assertion to fail. As far as I can see `TrappingMath` is not changed in
> the case FPExceptionBehavior is "ignore" or "maytrap".
> Clearly in the "ignore" case it should be safe to just set `TrappingMath` to
> false, but I'm not sure about the "maytrap" case.
> It seems that `-ffp-exception-behavior` is more general than
> `-f{,no-}trapping-math`, so it seems natural to me to see `ftrapping-math`
> and `foo-trapping-math` as aliases for `ffp-exception-behavior=strict` and
> `ffp-exception-behavior=ignore` respectively. If we agree on this, then I
> would expect the reasoning inside the compiler only in terms of
> `FPExceptionBehavior`.
Thanks for pointing out this assertion failure, I will upload a patch with fix.
Yes we could entirely express ftrapping-math and fno-trapping-math via the
ffp-exception-behavior= option. That would probably be better--currently the
trapping option becomes effective via the exception behavior parameter to the
llvm floating point constrained intrinsics, and it can take 3 values. I thought
it would be too radical at the moment, so I didn't propose that in this patch.
In the patch I'm about to add, I added a test case for the assertion that you
saw.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2576
+ CmdArgs.push_back("-ftrapping-math");
+ } else if (TrappingMathPresent)
CmdArgs.push_back("-fno-trapping-math");
----------------
michele.scandale wrote:
> With this change if I run `clang -### -ffast-math test.c` I don't see
> `-fno-trapping-math` passed to the CC1.
> This is changing the changes the value of the function level attribute
> "no-trapping-math" (see lib/CodeGen/CGCall.cpp : 1747).
> Is this an intended change?
>
> Moreover since with this patch the default value for trapping math changed,
> the "no-trapping-math" function level attribute is incorrect also for default
> case.
Before this patch, ftrapping-math was added to the Driver and also a bitfield,
``NoTrappingFPMath`` was created in the LLVM to describe the state of
trapping-math, but otherwise that bit wasn't consulted and the option had no
effect. Gcc describes ftrapping-math as the default, but in llvm by default
floating point exceptions are masked and this corresponds to the floating point
Constrained Intrinsics having exception behavior set to ignored. This patch
changed the llvm constructor to set the trapping bit to "no trap". In fact I'd
like to get rid of the ``NoTrappingFPMath`` bitfield since it's not being used,
but I didn't make that change at this point.
If I remember correctly, there are a bunch of driver tests that failed if
fno-trapping-math is output to cc1. I'd have to reconstruct the details. Since
fno-trapping-math is the default, it isn't passed through on the cc1 command
line: the Clang.cpp driver doesn't pass through the positive and negative for
each existing option.
Thanks for pointing out the line in CGCall.cpp, it seems the CodeGenOpts aren't
getting set up perfectly I'll fix that in CompilerInvocation.cpp; I don't see
anything setting trapping-math as part of function level attribute,
@michele.scandale did I overlook that/can you point out where that is?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62731/new/
https://reviews.llvm.org/D62731
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
