This is an archive of the discontinued LLVM Phabricator instance.

Fix CC1 command line options mapping into fast-math flags.
ClosedPublic

Authored by michele.scandale on May 20 2020, 10:39 AM.

Details

Summary

This fixes the mapping between CC1 command line options to the
properties in LangOptions describing the floating point optimizations
configuration.
The default fast-math flags for the IR builder are now derived from such
properties to avoid inconsistencies.
Finally some of the CodeGenOptions floating point optimizations
properties have been removed since they now exist in LangOptions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 10:39 AM
michele.scandale marked an inline comment as done.May 20 2020, 10:49 AM
michele.scandale added inline comments.
clang/test/CodeGenOpenCL/relaxed-fpmath.cl
14

This change is based on the following:

  • -cl-fast-relaxed-math = -cl-unsafe-math-optimizations + -cl-finite-math-only
  • the GCC option -funsafe-math-optimizations and -cl-unsafe-math-optimizations are described with very similar wording and from the GCC description states explicitly mention that no signed zeros, reassociation and reciprocals are enabled, but there is no mention to assuming that NaNs do not exist.

See https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/clBuildProgram.html and https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

michele.scandale marked 2 inline comments as done.May 20 2020, 10:52 AM
michele.scandale added inline comments.
clang/test/CodeGen/libcalls.c
11–21

For CUDA the default FP contract mode is fast, therefore the contract FMF is emitted.

clang/test/CodeGenCUDA/builtins-amdgcn.cu
13

For CUDA the default FP contract mode is fast, therefore the contract FMF is emitted.

Fix 'clang/test/CodeGenCUDA/library-builtin.cu'

Thanks for cleaning this up!

Fix formatting issues.

Anastasia added inline comments.May 21 2020, 1:13 PM
clang/test/CodeGenOpenCL/relaxed-fpmath.cl
14

Makes sense!

michele.scandale edited the summary of this revision. (Show Details)May 21 2020, 4:58 PM

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

Most of them yes.

In the CUDA test we there is now contract because we honor the default contraction mode that for CUDA is set to fast.

In clang/test/CodeGen/complex-math.c I've added -ffp-contract=fast to the command line option because -ffast-math at the CC1 level does not change the default contraction mode. We might want to treat -ffast-math similarly to -cl-fast-relaxed-math, i.e. it implies by default the "fast" contraction mode. Before this change there behavior was accidentally the same as "-ffast-math changes the default contraction mode" because there was:

if (CGM.getLangOpts().FastMath)
  FMF.setFast()

and so the bit for AllowContract was enabled in the fast-math flags.

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

Most of them yes.

In the CUDA test we there is now contract because we honor the default contraction mode that for CUDA is set to fast.

Right, and we weren't honoring that mode before?

In clang/test/CodeGen/complex-math.c I've added -ffp-contract=fast to the command line option because -ffast-math at the CC1 level does not change the default contraction mode.

Oh, I see, makes sense. So there was inconsistent treatment of the options, where one thing observed it but others didn't, and that's been fixed now.

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

Most of them yes.

In the CUDA test we there is now contract because we honor the default contraction mode that for CUDA is set to fast.

Right, and we weren't honoring that mode before?

Not in the setup of base fast-math flags inside CodeGenFunction. However when emitting code for expression with FPOptions stored in the AST nodes then contract was set correctly.

In clang/test/CodeGen/complex-math.c I've added -ffp-contract=fast to the command line option because -ffast-math at the CC1 level does not change the default contraction mode.

Oh, I see, makes sense. So there was inconsistent treatment of the options, where one thing observed it but others didn't, and that's been fixed now.

Do you think we should handle -ffast-math as -cl-fast-relaxed-math, i.e. it implies the default contraction mode being "fast"?

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

Most of them yes.

In the CUDA test we there is now contract because we honor the default contraction mode that for CUDA is set to fast.

Right, and we weren't honoring that mode before?

Not in the setup of base fast-math flags inside CodeGenFunction. However when emitting code for expression with FPOptions stored in the AST nodes then contract was set correctly.

Okay, that seems justifiable.

In clang/test/CodeGen/complex-math.c I've added -ffp-contract=fast to the command line option because -ffast-math at the CC1 level does not change the default contraction mode.

Oh, I see, makes sense. So there was inconsistent treatment of the options, where one thing observed it but others didn't, and that's been fixed now.

Do you think we should handle -ffast-math as -cl-fast-relaxed-math, i.e. it implies the default contraction mode being "fast"?

I'm actually surprised it doesn't. I can't imagine why someone enabling fast math would want contraction to be disabled.

The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably?

Most of them yes.

In the CUDA test we there is now contract because we honor the default contraction mode that for CUDA is set to fast.

Right, and we weren't honoring that mode before?

Not in the setup of base fast-math flags inside CodeGenFunction. However when emitting code for expression with FPOptions stored in the AST nodes then contract was set correctly.

Okay, that seems justifiable.

In clang/test/CodeGen/complex-math.c I've added -ffp-contract=fast to the command line option because -ffast-math at the CC1 level does not change the default contraction mode.

Oh, I see, makes sense. So there was inconsistent treatment of the options, where one thing observed it but others didn't, and that's been fixed now.

Do you think we should handle -ffast-math as -cl-fast-relaxed-math, i.e. it implies the default contraction mode being "fast"?

I'm actually surprised it doesn't. I can't imagine why someone enabling fast math would want contraction to be disabled.

Just to be clear the clang driver does the right thing.
If you run clang -ffast-math the CC1 invocation has both -ffast-math and -ffp-contract=fast (and other options as well)

Here specifically I'm just considering the behavior of clang -cc1 -ffast-math.

Rebase + -ffast-math change the default contraction mode to fast.

Fixup more tests with now redundant -ffp-contract=fast option.

rjmccall accepted this revision.May 29 2020, 12:32 PM

I'm actually surprised it doesn't. I can't imagine why someone enabling fast math would want contraction to be disabled.

Just to be clear the clang driver does the right thing.
If you run clang -ffast-math the CC1 invocation has both -ffast-math and -ffp-contract=fast (and other options as well)

Here specifically I'm just considering the behavior of clang -cc1 -ffast-math.

Okay. It is probably best for testing purposes if flags like this stay as close as possible to their driver behavior. The driver can just canonicalize different spellings down to the interface that -cc1 wants.

This revision is now accepted and ready to land.May 29 2020, 12:32 PM
michele.scandale requested review of this revision.EditedMay 29 2020, 2:42 PM

I've just realized it might be incorrect to have the CC1 option -ffast-math changing the default contraction mode. The clang driver generates -ffast-math based on conditions that do not involve the contraction mode state at all:

// -ffast-math enables the __FAST_MATH__ preprocessor macro, but check for the
// individual features enabled by -ffast-math instead of the option itself as
// that's consistent with gcc's behaviour.
if (!HonorINFs && !HonorNaNs && !MathErrno && AssociativeMath &&
    ReciprocalMath && !SignedZeros && !TrappingMath && !RoundingFPMath) {
  CmdArgs.push_back("-ffast-math");
  if (FPModel.equals("fast")) {
    if (FPContract.equals("fast"))
      // All set, do nothing.
      ;
    else if (FPContract.empty())
      // Enable -ffp-contract=fast
      CmdArgs.push_back(Args.MakeArgString("-ffp-contract=fast"));
    else
      D.Diag(clang::diag::warn_drv_overriding_flag_option)
        << "-ffp-model=fast"
        << Args.MakeArgString("-ffp-contract=" + FPContract);
  }
}

For example the running the following clang -### -funsafe-math-optimizations -ffinite-math-only -x c - lead to a CC1 command line without any -ffp-contract= option relying on the fact that the default value for the contraction mode in the compiler is OFF.

I will revert the modification on this aspect.

michele.scandale planned changes to this revision.May 29 2020, 2:48 PM

Revert last change about -ffast-math imply "fast" contraction mode by default in CC1.
Rebase.

Per a private discussion, it seems like the right thing to do here would be stop recognizing -ffast-math flag in cc1 and just put the driver in charge of all these individual flags. That may necessitate adding a -fdefine-fast-math cc1 option to control the #define, but the logic for that seems complex enough that it ought to be left to the driver.

Separately, we should consider making some more driver options imply -ffp-contract=fast. Fast contraction is the default behavior in GCC, and while Clang has chosen not to follow that precedent, it seems to be in keeping with the spirit of options like -funsafe-math-optimizations that they should broadly enable contraction if that policy isn't otherwise specified.

Those sorts of changes should be kept separate from this kind of refactor, though. This is still approved.

rjmccall accepted this revision.May 29 2020, 5:51 PM
This revision is now accepted and ready to land.May 29 2020, 5:51 PM

Thanks John.

Would you be able to land this on my behalf?