[PATCH] D28883: DAG: Recognize no-signed-zeros-fp-math attribute

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Target/TargetMachine.cpp:91
+  if (Options.UnsafeFPMath) {
+    // Should this imply the others?
+    Options.NoSignedZerosFPMath = true;
----------------
arsenm wrote:
> jlebar wrote:
> > jlebar wrote:
> > > Personally I'd prefer to do this in a separate patch, and to make a reasonable attempt at updating our documentation to say that this implies the others.  It was only after working on these flags for a few weeks that I even realized that UnsafeFPMath implies the others in IR (and therefore that it should imply the others elsewhere).
> > > 
> > > I even went as far as to add a separate unsafe-fp-math flag to XLA separate from the fast-math flag.  Oops.  :)
> > Oh, I see why you're doing it here -- you don't want to regress existing codegen that sets UnsafeFPMath but not this flag (which is new).
> > 
> > I guess you have to do it this way.  We should still do this with the intent to update the docs and set the other flags, though.
> I would actually prefer to someday split unsafe math into an unsafe algebra flag which does not imply the others for places where you can reassociate without changing nan/inf behavior.
> 
> Oddly, I checked what clang does for just  -funsafe-math-optimizations. It sets unsurprisingly:
> "unsafe-fp-math"="true"
> "no-signed-zeros-fp-math"="true" 
> "no-trapping-math"="true" 
> 
> But I was surprised it still emits:
> "less-precise-fpmad"="false"
> "no-infs-fp-math"="false" 
> "no-nans-fp-math"="false"
> 
> but these are enabled by -ffast-math. So from a practical point of view in the current world it doesn't really need to imply it, but it also confuses me about what unsafe-fp-math is really supposed to mean if before it was a stand in for no-signed-zeros.
> I would actually prefer to someday split unsafe math into an unsafe algebra flag which does not imply the others for places where you can reassociate without changing nan/inf behavior.

I mean, me too.  At least, I *think* I want to be able to reassociate / lose precision without assuming that I never produce a NaN or Inf.  I *think* that's rather useful, but maybe someone can learn me why it's not.

And clearly the flags are a mess, yikes.

But at an IR level, the only way to set "unsafe-fp-math" on an instruction is with the "fast" attr, and that implies all the the things.  I'm also not a fan of pretending that we have more configurability than we actually do...


https://reviews.llvm.org/D28883