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

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

In D79187#2013528 <https://reviews.llvm.org/D79187#2013528>, @efriedma wrote:

> (Alive2 is happy with the transform; see http://volta.cs.utah.edu:8080/z/oR3wKj .)
>
> > I did find it hard to get the fold conditions correct to avoid infinite loops in the AArch64 backend (which affected fp16 types),
>
> Can you give an example here?
>
> We probably need some code to avoid creating illegal SINT_TO_FP after legalization; that could lead to an infinite loop.  There are lots of examples in DAGCombine to follow that call isOperationLegal.


This patch itself does not cause infinite loops. However, small changes to the validity checker of the transform seem to cause issues, in the worst case breaking all of the following:

  LLVM :: CodeGen/AArch64/arm64-convert-v4f64.ll
  LLVM :: CodeGen/AArch64/arm64-fast-isel-conversion-fallback.ll
  LLVM :: CodeGen/AArch64/complex-int-to-fp.ll
  LLVM :: CodeGen/AArch64/f16-instructions.ll
  LLVM :: CodeGen/AArch64/fdiv_combine.ll
  LLVM :: CodeGen/AArch64/fp16-v16-instructions.ll
  LLVM :: CodeGen/AArch64/fp16-v4-instructions.ll
  LLVM :: CodeGen/AArch64/fp16-v8-instructions.ll

I did try to ensure that SINT_TO_FP was legal, but I'm not sure the query I had for that was ever correct, because I'm not sure the query `hasOperation(ISD::SINT_TO_FP, IntTy)` necessarily tells you anything - I think I'm looking for the other half of this query, which says "is it legal to go *to* this FP type with `sint_to_fp`. Guidance on this would also be helpful :) I'm new to the DAGCombiner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79187