Page MenuHomePhabricator

[ValueTracking] Strengthen impliesPoison reasoning
ClosedPublic

Authored by nikic on Jan 16 2021, 8:24 AM.

Details

Summary

This implements the same basic change as D94859, but in a more general way. directlyImpliesPoison recurses on V, impliesPoison recurses on ValAssumedPoison. Both recursions have independent small depth limits. This allows us to handle more than just the icmp+icmp case, such as the mask+icmp case for example.

Diff Detail

Event Timeline

nikic requested review of this revision.Jan 16 2021, 8:24 AM
nikic created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2021, 8:24 AM

Okay, I agree that this is more general.
I was cautious about possible compile time degradation - do you have a number for this?

unittests/Analysis/ValueTrackingTest.cpp
783 ↗(On Diff #317179)

Maybe icmp eq i32 %M2, 1 here? (similarly below)

790 ↗(On Diff #317179)

Shall we have one more example that has EXPECT_FALSE because ValAssumedPoison was e.g. icmp eq %x, %z where z wasn't used by A?

nikic updated this revision to Diff 317378.Jan 18 2021, 9:30 AM
nikic marked an inline comment as done.

Fix typos in mask test.

nikic added a comment.Jan 18 2021, 9:59 AM

Okay, I agree that this is more general.
I was cautious about possible compile time degradation - do you have a number for this?

Compile-time results (on top of flipping the select optimization flag, otherwise it would not get called): https://llvm-compile-time-tracker.com/compare.php?from=0c9e135ecde32b57393fb502cc45f90c2a17346d&to=3d7dcc78542627d6440613327f21d177bcdf6f00&stat=instructions There does not seem to be any impact above noise.

unittests/Analysis/ValueTrackingTest.cpp
783 ↗(On Diff #317179)

Yes, this test didn't make sense...

790 ↗(On Diff #317179)

I think this case is checked by the existing impliesPoisonTest_ICmpUnknown test. It uses icmp eq i32 %x, %y and icmp eq i32 %x, 1.

aqjune accepted this revision.Jan 18 2021, 2:20 PM

LGTM

unittests/Analysis/ValueTrackingTest.cpp
790 ↗(On Diff #317179)

+1

This revision is now accepted and ready to land.Jan 18 2021, 2:20 PM
This revision was automatically updated to reflect the committed changes.