[PATCH] D48085: [DAGCombiner] restrict (float)((int) f) --> ftrunc with no-signed-zeros

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Should this adjust the ReleaseNotes?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11049-11050
+  // replacing casts with a libcall. We also must be allowed to ignore -0.0
+  // because FTRUNC will return -0.0 for (-1.0, -0.0), but using integer
+  // conversions would return +0.0.
+  // FIXME: We should be able to use node-level FMF here.
----------------
spatel wrote:
> lebedev.ri wrote:
> > spatel wrote:
> > > lebedev.ri wrote:
> > > > Couldn't we do `and (ftrunc %x), (-1 >> 1)`, i.e. unset the sign bit?
> > > > Or is that worse than not transforming to `ftrunc` in the first place?
> > > You'd want to keep this in FP, so I think that would be:
> > > fabs (ftrunc x)
> > > 
> > > And that could be better than the alternative in some cases. See D44909 for what this looks like without using ftrunc. If the sequence is more than 2 converts, fabs is likely a winner.
> > > 
> > > But given the fallout from the original patch, I expect this is the patch most people would prefer. :)
> > > We'll avoid the UB headaches for most code, and code that cares about FP perf likely has some loosened FP constraints anyway.
> > > You'd want to keep this in FP, so I think that would be:
> > > fabs (ftrunc x)
> > Right, right. Otherwise you might need to leave fp domain, etc etc.
> > 
> > > But given the fallout from the original patch, I expect this is the patch most people would prefer. :)
> > That cat is already out of the bag though, and modulo this edge-case, the transform is valid i think.
> > Backtracking like this, while not for no reason, may be viewed as acceptance of views of some who
> > think UB is bad and lack of UB [in code] should not be assumed by compilers. But this is purely my opinion.
> > 
> Depends on your perspective. For brave (much appreciated!) projects that track LLVM trunk, yes they've already suffered/benefited. But most potential customers won't see this change until the next major release.
> 
> But this discussion is independent of this particular patch. We have a miscompile that must be fixed. Salvaging perf is secondary, and I don't think we can do it universally - ie, converting 2 casts to fabs+ftrunc or select+fcmp+fabs+ftrunc requires a TLI hook, so that's definitely a bigger patch than what we have here currently.
Could you please at least add a `FIXME`? :)


https://reviews.llvm.org/D48085