[PATCH] D31182: [InstCombine] fadd double (sitofp x), y check that the promotion is valid

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D31182#707598, @apilipenko wrote:

> In https://reviews.llvm.org/D31182#706365, @spatel wrote:
>
> > Before getting to any details about the patch, we need to address the question raised in PR27036: why are we doing this transform in InstCombine at all? The assumption is that an integer add is more canonical and/or cheaper than an FP add. Is that universally true?
>
>
> It's difficult to tell. Even if FP addition is cheaper the second combine also saves us one sitofp conversion. It should be also taken into account.
>
> These combines are buggy and there is a simple fix. Since I don't have a good understanding on the profitability of these combines I decided to go ahead with the fix rather then removing them for example.


Agreed - solving the miscompiling is the most important thing. Since we can do that quickly with a small patch, let's go ahead. We can decide what is canonical form as a next step, but please add a 'TODO' comment about that.

This example leads into the same gray area we have with several instcombine folds: it's not clear if the value-track-ability is enough to justify the transform, whether a backend should be responsible for fixing something that is not optimal for all targets, if the removal of an IR instruction overrides either of those...


https://reviews.llvm.org/D31182