[PATCH] D46322: [SelectionDAG] propagate 'afn' and 'reassoc' from IR fast-math-flags

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

I realize that you're abandoning this, in favor of simplifying https://reviews.llvm.org/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.


https://reviews.llvm.org/D46322