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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D117302#3248112 <https://reviews.llvm.org/D117302#3248112>, @kovdan01 wrote:

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

I think it would start from `InstCombinerImpl::visitFSub()` and be very similar to the specialization that already exists as `foldFNegIntoConstant`. I already drafted a patch for it while looking at this, so I can clean that up and post it (the harder part is adding a pile of tests to check FMF propagation!).

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

I agree. I'm still not sure exactly what happens within `FAddCombine`, but this patch just removes code, so LGTM.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:540
+    // desirable to reside at the top of the resulting expression tree. Placing
+    // constant close to supper-expr(s) will potentially reveal some
+    // optimization opportunities in super-expr(s). Here we do not implement
----------------
typo: supper -> super


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

https://reviews.llvm.org/D117302