[PATCH] D32596: [DAGCombine] Transform (fadd A, (fmul B, -2.0)) -> (fsub A, (fadd B, B)).

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D32596#745262, @mcrosier wrote:

> In https://reviews.llvm.org/D32596#745247, @gberry wrote:
>
> > My only comment is that you may be missing additional cases where the fneg could be folded away, but perhaps those can be fixed in a follow up change.
>
>
> Yes, I'll investigate once this patch lands.


Seems like isNegatibleForFree() would be the place to recognize patterns like this, and then we'd have a corresponding special case for -2.0 in GetNegatedExpression().

As written, this transform should be good for all x86 because it removes a constant load, so no objections, but...
I'm confused about our handling of FP folds. We're saying that this is a universally good (all targets and no relaxed FP needed) codegen fold, but we don't want it in IR/InstCombine because we prefer constants there.

Some tests to think about below. We'll fold the first 3 in DAGCombiner after this patch (universally afaict), but InstCombine does nothing with those.  Should InstCombine fold fnegs into constants and fsub -> fadd?

The last case is transformed partially in InstCombine (div -> mul), but DAGCombiner does nothing with that. It's ok to not have a DAG fold for that because nothing this late is producing an fdiv?

  define float @add_mul_neg2(float %a, float %b) {
    %mul = fmul float %b, -2.0
    %add = fadd float %a, %mul
    ret float %add
  }
  
  define float @sub_mul_neg2(float %a, float %b) {
    %mul = fmul float %b, -2.0
    %sub = fsub float %a, %mul
    ret float %sub
  }
  
  define float @mul_mul_neg2(float %a, float %b) {
    %mul = fmul float %b, -2.0
    %neg = fsub float -0.0, %a
    %mul2 = fmul float %neg, %mul
    ret float %mul2
  }
  
  define float @sub_div_neghalf(float %a, float %b) {
    %div = fdiv float %b, -0.5
    %sub = fsub float %a, %div
    ret float %sub
  }


https://reviews.llvm.org/D32596