Page MenuHomePhabricator

[ValueTracking] improve analysis for "C << X" and "C >> X"
ClosedPublic

Authored by spatel on Feb 3 2021, 9:49 AM.

Details

Summary

This is based on the example/comments in:
https://llvm.org/PR48984

I tried just lifting the restriction in computeKnownBitsFromShiftOperator() as suggested, but that doesn't catch all of the cases shown here. I didn't step through to see exactly why that happened. But it seems like a reasonable compromise to cheaply check the special-case of shifting a constant.

There's a slight regression on a cmp transform as noted, but I think this is likely the more important/common pattern, so we can fix that icmp pattern later if needed.

Diff Detail

Event Timeline

spatel created this revision.Feb 3 2021, 9:49 AM
spatel requested review of this revision.Feb 3 2021, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 9:49 AM

This is very similar to D90479 which is trying to do this more generally by using the KnownBits shift helpers.

This is very similar to D90479 which is trying to do this more generally by using the KnownBits shift helpers.

Ah, I forgot about that patch. Was anything holding it up?
Yes, they're showing almost the same test diffs apart from the zext+icmp test. I don't have a preference for the fix, so we should push one of these.

This is very similar to D90479 which is trying to do this more generally by using the KnownBits shift helpers.

Ah, I forgot about that patch. Was anything holding it up?
Yes, they're showing almost the same test diffs apart from the zext+icmp test. I don't have a preference for the fix, so we should push one of these.

D90479 noticed that we were assuming a poison shift could be 'known' to be all zero - so I needed to go back and fix that first but I never did (got distracted by reductions, and other squirrels.....).

If you want to get this patch that's fine - D90479 will just remove this again when I get around to looking at it.

spatel added a comment.Feb 8 2021, 9:10 AM

If there are no objections (and someone approves), I can push this change to make progress.

RKSimon accepted this revision.Feb 8 2021, 1:29 PM

LGTM

This revision is now accepted and ready to land.Feb 8 2021, 1:29 PM
This revision was automatically updated to reflect the committed changes.