[PATCH] D44909: [DAGCombine] (float)((int) f) --> ftrunc (PR36617)

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In https://reviews.llvm.org/D44909#1065109, @spatel wrote:

> In https://reviews.llvm.org/D44909#1065089, @thakis wrote:
>
> > This broke some large integration test on arm64 in Chromium (https://crbug.com/831145 – doesn't yet have any information other than "something is broken" and that we bisected it to this revision). It sounds like the patch got rewritten during review; does "formally allowed by the standard, but extremely likely to break things and surprise people" still apply? It sounds like this is supposed to be behavior-preserving for values that aren't NaN and Inf, but it does change behavior for those two? (If so, in what way?)
>
>
> The patch started off as an IR transform, and the quoted comment applied only to cases (I hope) where the target doesn’t have native support for an fptrunc instruction. NaN/inf inputs are undefined with these ops, so if something was relying on that undefined output, that could be the source of the problem.
>
> I’m not an arm64 expert, but the tests show that we should codegen ‘frintz’ instructions with this patch. So that would be the place to look for diffs.


We found it: https://bugs.chromium.org/p/chromium/issues/detail?id=831145#c48

It's exactly the "formally allowed by the standard, but extremely likely to break things and surprise people" situation :-) The question is just what the appropriate fix is...


Repository:
  rL LLVM

https://reviews.llvm.org/D44909