[PATCH] D117302: [InstCombine] Simplify addends reordering logic

[Daniil Kovalev via Phabricator via llvm-commits]

kovdan01 added a comment.

In D117302#3246034 <https://reviews.llvm.org/D117302#3246034>, @spatel wrote:

> I can't tell if this has a greater effect than shown by the test diff. That diff actually demonstrates a basic missing canonicalization in IR - no fast-math is needed to convert fsub to fadd in that example or any of these more general cases:
>
>   define float @fmul_c1(float %x, float %y) {
>     %m = fmul float %x, 7.000000e+00
>     %r = fsub float %y, %m
>     ret float %r
>   }
>   
>   define float @fdiv_c0(float %x, float %y) {
>     %m = fdiv float 7.000000e+00, %x
>     %r = fsub float %y, %m
>     ret float %r
>   }
>   
>   define float @fdiv_c1(float %x, float %y) {
>     %m = fdiv float %x, 7.000000e+00
>     %r = fsub float %y, %m
>     ret float %r
>   }
>
> We do have this transform in the backend though (pushing the implicit negation op into the constant operand). For example, if you codegen this with llc for x86, you'll see 3 "addss" instructions and no "subss". Would adding those canonicalizations to IR solve your motivating case(s)? 
> Or is there a larger example of a change resulting from this patch?

Thanks for your comment! I don’t have a larger example that results from this patch and adding the canonicalization that you mentioned will solve my motivation case. I can implement that and submit as a separate patch if that’s applicable for you. Could you please tell me the right place to add the canonicalization to?

Getting back to the current patch – I believe that it is also worth to be merged – at least, in terms of refactoring (having code that is intended to do the same thing in two places looks weird IMHO).


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

https://reviews.llvm.org/D117302