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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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:
> > 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.



https://reviews.llvm.org/D48085