Page MenuHomePhabricator

Fix interference caused by fmul 2, x -> fadd x, x combine
ClosedPublic

Authored by arsenm on Jul 25 2014, 3:56 PM.

Details

Reviewers
hfinkel
Summary

This solves 2 variants of this problem. First, change the order things are tried so that fmul (fmul x, c1) c2 -> fmul x, (fmul c1, c2) before fadd x, x.

Also add a variant of the fmul constant combine that understands fadd x, x as a multiply by 2. This is necessary because a multiply by 2 that exists originally will be transformed into the fadd by one of the early runs of DAG combiner, and not folded with new fmuls inserted during lowering.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 11901.Jul 25 2014, 3:56 PM
arsenm retitled this revision from to Fix interference caused by fmul 2, x -> fadd x, x combine .
arsenm updated this object.
arsenm edited the test plan for this revision. (Show Details)
arsenm added a subscriber: Unknown Object (MLST).
arsenm updated this revision to Diff 11903.Jul 25 2014, 3:58 PM

Add more tests I forgot to attach

hfinkel added a subscriber: hfinkel.Sep 2 2014, 9:53 AM
hfinkel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
691

BV can only truncate integer operands, not FP ones. If you want to check that the types agree, please make it an assert.

6863

As a general note, it looks like we don't currently preserve FastMathFlags at the SDAG level. We've fixed that for NSW/NUW, and we should probably fix that for FP ops too (obviously this is a separate issue from what you're addressing here).

6875

Okay, but what happens next? Wouldn't we get (fmul x, (fadd c, c))?

Thanks Hal!
Committed revision 216913.

Forget my last comment... Posted in the wrong code review :-(

arsenm added inline comments.Sep 2 2014, 11:28 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
691

This was already added in another patch which also used this. I can fix this separately

6875

The fmul 2.0, c will be constant folded into 2c which is the goal

hfinkel accepted this revision.Sep 2 2014, 11:31 AM
hfinkel added a reviewer: hfinkel.

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
691

Okay, thanks! (please do).

6875

Okay, makes sense. ;)

This revision is now accepted and ready to land.Sep 2 2014, 11:31 AM
arsenm closed this revision.Sep 2 2014, 12:12 PM

r216932 and 216928