[PATCH] D141419: [DAGCombine] Suppress some foldings of rounding to fp16

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

I'll re-work this to handle it in the lowering, as @lebedev.ri suggested.  And take the other comments into account then.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16274-16275
+    // from f32.  Suppress this folding in both the scalar and vector forms.
+    bool TargIsFp16 = VT == MVT::f16 ||
+                      (VT.isVector() && VT.getVectorElementType() == MVT::f16);
+    if (TargIsFp16) {
----------------
pengfei wrote:
> This equals to `VT.getScalarType() == MVT::f16`.
Thanks @pengfei.  (I should have seen that.)


================
Comment at: llvm/test/CodeGen/X86/fastmath-float-half-conversion.ll:68
+; With 'unsafe-fp-math', it is legal to fold this to converting directly
+; from 'double' -> 'half'.  But that is generally less efficient (done
+; via slow run-time calls) than the two-step unfolded approach.  So
----------------
pengfei wrote:
> It's still efficient to in the AVX case. Calling to single run-time is better than `vcvtsd2ss` + `__truncsfhf2`.
Good point.  When I re-work this to handle things in the lowering, I'll take that into account.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141419/new/

https://reviews.llvm.org/D141419