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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D44909#1065600, @hans wrote:

> 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...


Aha - nice detective work. :)
If it helps, I can revert this while fixing the sources. Can this pattern be added to the UB sanitizer? Or maybe it's already there, but wasn't used?


Repository:
  rL LLVM

https://reviews.llvm.org/D44909