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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked an inline comment as done.
arsenm added inline comments.


================
Comment at: lib/Target/TargetMachine.cpp:91
+  if (Options.UnsafeFPMath) {
+    // Should this imply the others?
+    Options.NoSignedZerosFPMath = true;
----------------
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.


https://reviews.llvm.org/D28883