[PATCH] D79187: [DAGCombiner] fold (fp_round (int_to_fp X)) -> (int_to_fp X)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> However, small changes to the validity checker of the transform seem to cause issues

You mean, we don't have enough test coverage to catch all the issues? :)

Actually, I guess you won't run into issues with the current version of the patch because the legal integer types aren't narrow enough.  I assume if you added a call to GetNumSignBits or whatever, the issues would pop back up.  The problem is essentially that the fp16 type is "legal", but doesn't have SINT_TO_FP; we instead convert to a 32-bit float, then truncate the float to fp16.  Which is exactly the pattern you're matching here.

I think if the result type of the fp_round is a half, the transform is legal for any integer type.  half is so tiny that all integer half values fit into the mantissa of a 32-bit float, so there can't be any rounding issues.  Alive2 agrees.

> I'm not sure the query hasOperation(ISD::SINT_TO_FP, IntTy) necessarily tells you anything

If that returns Legal, it means SINT_TO_FP is legal for all legal floating-point types.  If it's not legal for some types, the target has to mark it Custom and add handling for the types in question.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79187