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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a comment.

In https://reviews.llvm.org/D32596#745330, @spatel wrote:

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


I'll investigate this suggestion. Thanks.

> As written, this transform should be good for all x86 because it removes a constant load, so no objections, but...

Okay, good.

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

As Eli pointed out, the reassociation pass prefers the fmul with constant (to expose additional factoring opportunities, IIRC).  He also pointed out that we should prefer fmul X, 2.0 as the canonical form, for the same reason we prefer "shl %x, 1" over "add %x, %x": we optimize instructions where hasOneUse() is true more aggressively.  Given these two implications I went ahead and implemented this as a DAG combine.  However, I think it might make sense to have InstCombine canonicalize to the mul with constant form as well.

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

While InstCombine does nothing, the reassociation pass will canonicalize to

  (fsub A, (fmul/fdiv B, -C)) -> (fadd A, (fmul/fdiv B, C))
  (fadd A, (fmul/fdiv B, -C)) -> (fsub A, (fmul/fdiv B, C))

where C is a constant and

  (fsub A, (fdiv b, -0.5)) -> (fadd A, (fmul b, 2.0))

for the last test.

We could make InstCombine prefer these forms as well and that might make a difference, but it might not.  My guess is it probably doesn't matter, but I'll experiment.


Repository:
  rL LLVM

https://reviews.llvm.org/D32596