Page MenuHomePhabricator

[SelectionDAG] propagate 'afn' and 'reassoc' from IR fast-math-flags
AbandonedPublic

Authored by spatel on May 1 2018, 9:53 AM.

Details

Summary

This is extracted/simplified from D45710 because I think we should break that into smaller pieces and have tests for each step. If nobody else thinks that's necessary, I'll abandon.

Note that I'm proposing that we go without adding an 'isFast()' umbrella in the DAG to avoid that misuse entirely. This means that the relatively few current folds that use SDNodeFlags may have bugs where we are overstepping the bounds of 'reassoc' alone.

But that's ok because these folds are also checking TargetOptions.UnsafeFPMath which isn't actually an umbrella flag based on its code comment:

/// UnsafeFPMath - This flag is enabled when the
/// -enable-unsafe-fp-math flag is specified on the command line.  When
/// this flag is off (the default), the code generator is not allowed to
/// produce results that are "less precise" than IEEE allows.  This includes
/// use of X86 instructions like FSIN and FCOS instead of libcalls.

I think this is intended to have the same meaning as 'reassoc' in IR:

reassoc
  Allow reassociation transformations for floating-point instructions. This may dramatically change results in floating-point.

So this flag can be set independently of NoSignedZerosFPMath for example, and we're supposed to be restricting folds based on that possibility. If not, then we already have a bug (most likely with handling of -0.0).

To fix that, we need to audit each transform (as we've been doing in IR) to see exactly which FMF are required for each transform.

So in summary, this patch is intended to be no more wrong than we already may be. It just seeks to unify the flags between IR and the DAG.

Diff Detail

Event Timeline

spatel created this revision.May 1 2018, 9:53 AM

I realize that you're abandoning this, in favor of simplifying D45710, but there was one point here that I wanted to comment on. Specifically:

... This means that the relatively few current folds that use SDNodeFlags may have bugs where we are overstepping the bounds of 'reassoc' alone.

But that's ok because these folds are also checking TargetOptions.UnsafeFPMath which isn't actually an umbrella flag based on its code comment:

/// UnsafeFPMath - This flag is enabled when the
/// -enable-unsafe-fp-math flag is specified on the command line.  When
/// this flag is off (the default), the code generator is not allowed to
/// produce results that are "less precise" than IEEE allows.  This includes
/// use of X86 instructions like FSIN and FCOS instead of libcalls.

I think this is intended to have the same meaning as 'reassoc' in IR:
...

In practice, with the Clang front-end, this flag does mean more than just the 'reassoc' flag. In particular with Clang, '-enable-unsafe-fp-math' is specified on the cc1 command-line when the following user-level switches are specified:

-fno-math-errno -fassociative-math -freciprocal-math -fno-signed-zeros -fno-trapping-math

'reassoc' in IR can be true when (for example) allow-reciprocal ('arcp') is false. So (again, with the Clang front-end), the following command (which does not pass '-enable-unsafe-fp-math'):

-fno-math-errno -fassociative-math -fno-signed-zeros -fno-trapping-math

will set 'reassoc'.

So if I've understood this correctly, regarding:

So in summary, this patch is intended to be no more wrong than we already may be. It just seeks to unify the flags between IR and the DAG.

it's theoretically possible for this patch to be more aggressive (and so "be more wrong"). In practice, I don't think it will do harm because of the way these flags are currently used.

nhaehnle removed a subscriber: nhaehnle.May 3 2018, 2:59 AM
spatel abandoned this revision.May 3 2018, 6:34 AM

In practice, with the Clang front-end, this flag does mean more than just the 'reassoc' flag. In particular with Clang, '-enable-unsafe-fp-math' is specified on the cc1 command-line when the following user-level switches are specified:

-fno-math-errno -fassociative-math -freciprocal-math -fno-signed-zeros -fno-trapping-math

'reassoc' in IR can be true when (for example) allow-reciprocal ('arcp') is false. So (again, with the Clang front-end), the following command (which does not pass '-enable-unsafe-fp-math'):

-fno-math-errno -fassociative-math -fno-signed-zeros -fno-trapping-math

will set 'reassoc'.

Good point. I see this as even more reason to break up the steps of propagating/using the flags in the DAG and MI as much as possible and add tests along the way, so we have a better understanding of exactly what FMF semantics are in play. IOW, reduce the risk that we'll introduce mistakes as we update the plumbing.