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

[Warren Ristow via Phabricator via llvm-commits]

wristow added a comment.

> Sounds like the lowering should just be improved to do the opposite of this?

Thanks for the quick response @lebedev.ri.

So (focusing on the 'double' source) by that, do you mean splitting 'double -> half' into 'double -> float -> half'?  That isn't technically safe, although it could be done legally in "unsafe-fp-math" mode.  Ironically, that's what used to happen, but 02fe96b24018bb8ce65cb264e0621459507cf989 <https://reviews.llvm.org/rG02fe96b24018bb8ce65cb264e0621459507cf989> suppressed that feature.  We could modify the change of that commit to no longer suppress it in "unsafe-fp-math" mode.

FTR, that commit 02fe96b24018bb8ce65cb264e0621459507cf989 <https://reviews.llvm.org/rG02fe96b24018bb8ce65cb264e0621459507cf989> caused us trouble, in that we encountered user-code that essentially did:

  void test(double d, __fp16 *pFp16) {
    *pFp16 = __fp16(float(d));
  }

and that previously worked fine even though dagcombine folded this into a single rounding, `__fp16(d)`, because the lowering un-did that folding.  With that commit, this folding results in a call to `__truncdfhf2` for our target (and some others).  And we don't supply that routine in our run-time lib.  Seeing the comment in dagcombine:
//"f80 to f16 always generates an expensive (and as yet, unimplemented) libcall to __truncxfhf2"//
I was motivated to expand the check here to also handle the folding from `double` (and to have the code here also handle the vector versions).

Until your suggestion, I hadn't considered modifying the change of 02fe96b24018bb8ce65cb264e0621459507cf989 <https://reviews.llvm.org/rG02fe96b24018bb8ce65cb264e0621459507cf989> to allow it to do the transform when "unsafe-fp-math" is on.  Thinking about it now, that should also generate better code (when "unsafe-fp-math" is on) for the case where the user has explicitly written a conversion from `double` to `__fp16`.  So that seems better to me.  I'll do that, unless others come back with an argument to have this done in dagcombine.


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

https://reviews.llvm.org/D141419