This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] When merging conditional stores, don't count the store we're merging against the PHINodeFoldingThreshold
ClosedPublic

Authored by craig.topper on Nov 2 2017, 2:38 PM.

Details

Summary

Merging conditional stores tries to check to see if the code is if convertible after the store is moved. But the store hasn't been moved yet so its being counted against the threshold.

The patch adds 1 to the threshold comparison to make sure we don't count the store. I've adjusted a test to use a lower threshold to ensure we still do that conversion with the lower threshold.

Diff Detail

Event Timeline

craig.topper created this revision.Nov 2 2017, 2:38 PM
spatel edited edge metadata.

Adding Eli and Alexey as potential reviewers based on D36841 because I've never looked at this function before.

efriedma edited edge metadata.Nov 3 2017, 11:37 AM

What effect does this have on performance?

This had no affect on the binary output for any of the benchmarks we ran on trunk.

I only noticed this because one of our internal branches has diverged from community and changed the PHINodeFoldingThreshold from 2 to 1 (I wasn't involved in why that change was made in our branch). This lower threshold causes us to miss cases like merge-cond-store-2.ll despite the fact that it is if convertible with the threshold of 1 after the store is removed.

This revision is now accepted and ready to land.Nov 3 2017, 11:48 AM
This revision was automatically updated to reflect the committed changes.