This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Transform (A + -2.0*B*C) -> (A - (B+B)*C)
Needs ReviewPublic

Authored by Quolyk on Nov 8 2017, 11:06 PM.
This revision needs review, but all reviewers have resigned.

Details

Reviewers
mcrosier
Summary

This is my first attempt to contribute to llvm. I'm trying to implement this https://bugs.llvm.org/show_bug.cgi?id=32939. I'm struggling with writing tests for this patch. I will be very thankful if somebody guides me trough writing tests for such thing. Thanks a lot.

Diff Detail

Event Timeline

Quolyk created this revision.Nov 8 2017, 11:06 PM
fhahn added a subscriber: fhahn.Nov 9 2017, 7:39 AM
fhahn added a comment.Nov 9 2017, 7:50 AM

Thanks for working on this!

You could try to add a test case to test/CodeGen/AArch64/fadd-combines.ll for example, i.e. add a new function there with the fadd/fmul pattern you want to match.

mcrosier edited edge metadata.EditedNov 9 2017, 1:49 PM

In the bug report I provided a C test case:

double test2(double a, double b, double c) {
  return a + -2.0*b*c;
}

You can use this to create an IR test case using the following command:

clang test.c -O3 -s -emit-llvm -o test.ll

The IR should look something like this:

define double @test2(double %a, double %b, double %c) local_unnamed_addr #0 {
entry:
  %mul = fmul double %b, -2.000000e+00
  %mul1 = fmul double %mul, %c
  %add = fadd double %mul1, %a
  ret double %add
}

You can add your test case to the file suggested by Florian. That should also include examples of how to add the FILECHECK directives, etc.

escha added a subscriber: escha.Nov 9 2017, 2:29 PM

what is the purpose of this transform? why is the new form considered more canonical?

Quolyk updated this revision to Diff 122393.Nov 9 2017, 11:14 PM

Added some tests

Quolyk retitled this revision from [DAGCombine] [WIP] Transform (A + -2.0*B*C) -> (A - (B+B)*C) to [DAGCombine] Transform (A + -2.0*B*C) -> (A - (B+B)*C).Nov 9 2017, 11:41 PM

what is the purpose of this transform?

The general assumption is that a FP addition will be less expensive than a FP multiply.

why is the new form considered more canonical?

There was a bit of discussion in D32596 with respect to what is considered canonical form. Basically, in IR reassociation and inst combine prefer the 2*a version primarily because this results in 'a' having a single use, which we generally optimize more aggressively than multiple uses.

Is there a particular case you're concerned about?

mcrosier added inline comments.Nov 10 2017, 9:23 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9719

What if N1 is a constant? I suspect you'll run into an assertion as constants don't have operands.

This solution doesn't seem very general, it won't catch.

double test2(double a, double b, double c, double d) {
  return a + -2.0*b*c*d;
}

The constant can be many layers of multiplies away. Reassociate pushes constants down the tree. Should reassociate be pulling out the negate when it factors the tree?

mcrosier added a comment.EditedNov 10 2017, 11:09 AM

This solution doesn't seem very general, it won't catch.

double test2(double a, double b, double c, double d) {
  return a + -2.0*b*c*d;
}

The constant can be many layers of multiplies away. Reassociate pushes constants down the tree. Should reassociate be pulling out the negate when it factors the tree?

Reassociation prefers to "break up subtracts" by converting X-Y to X+-Y, so it can better commute operands to expose more opportunities to reassociate. It turns out that instcombine also prefers this form when Y is a constant. I'm not sure pulling out the negate would work unless we decide to change the canonical form throughout the pipeline, right?

Quolyk updated this revision to Diff 122613.Nov 13 2017, 12:32 AM

Code review. Fix if N1 is constant.

mcrosier resigned from this revision.Jan 23 2018, 6:39 AM