This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Remove redundant combine from visitAnd
ClosedPublic

Authored by craig.topper on Mar 31 2017, 10:50 AM.

Details

Summary

As far as I can tell this combine is fully handled by SimplifyDemandedInstructionBits.

I was only looking at this because it is the only user of APIntOps::isShiftedMask which is itself broken. As demonstrated by r299187. I was going to fix isShiftedMask and needed to make sure we had coverage for the new cases it would expose to this combine. But looks like we can nuke it instead.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 31 2017, 10:50 AM
spatel accepted this revision.Apr 2 2017, 8:23 AM

LGTM. If there's some way to annotate/update the tests that were originally added with that code, that would be nice.

My 'blame' skills aren't very good, so I'm not sure when this code was written. But it must have been pre-2010 when InstCombine was split off into new files.

This revision is now accepted and ready to land.Apr 2 2017, 8:23 AM
davide edited edge metadata.Apr 2 2017, 10:20 AM

I think this is fine. As Sanjay pointed out, can you please show us the test that performed this redundant transform if you can find it? I'd like to take a closer look.

I think the first version of this was added in r23379 in 2005, but there's no test case changes in that commit, but one is mentioned by name in the commit message. That test appears to be test29 in test/Transforms/InstCombine/add.ll that was added in the commit previous to that. That test case still exists today.

This revision was automatically updated to reflect the committed changes.
craig.topper reopened this revision.Apr 4 2017, 9:00 AM

I had to revert the commit of this due to a tsan check failure. it looks like this optimization does fire sometimes because it's mising a qualification to make sure the add or sub is only has one user. So we can create an add or sub without replacing the existing one.

This revision is now accepted and ready to land.Apr 4 2017, 9:00 AM
This revision was automatically updated to reflect the committed changes.