This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Don't do an unchecked shift in ComputeNumSignBits
ClosedPublic

Authored by sanjoy on Feb 23 2017, 2:04 PM.

Details

Summary

Previously we used to return a bogus result, 0, for IR like `ashr %val,
-1`.

I've also added an assert checking that ComputeNumSignBits at least
returns 1. That assert found an already checked in test case where we
were returning a bad result for ashr %val, -1.

Fixes PR32045.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy created this revision.Feb 23 2017, 2:04 PM
majnemer added inline comments.Feb 23 2017, 2:22 PM
lib/Analysis/ValueTracking.cpp
2209 ↗(On Diff #89562)

Shouldn't we treat this in the same way we treat Instruction::Shl?

sanjoy added inline comments.Feb 23 2017, 2:37 PM
lib/Analysis/ValueTracking.cpp
2209 ↗(On Diff #89562)

You mean return 1 for undefined shifts? I don't see a good reason why we should do that (both here and in shl) -- ashr ing more than the bitwidth is undef anyway, so why not be maximally aggressive?

why not be maximally aggressive?

This is not how undef works... unlike poison, you can't assert sign bits you don't actually have.

why not be maximally aggressive?

This is not how undef works... unlike poison, you can't assert sign bits you don't actually have.

Not sure I fully understand you, but I interpreted ashr 's "If op2 is (statically or dynamically) equal to or larger than the number of bits in op1, the result is undefined" rule as meaning ashr i32 %V, 32 is undef. This would mean I can pretend ashr i32 %V, 32 is 0, which has 32 sign bits.

majnemer added inline comments.Feb 23 2017, 4:43 PM
lib/Analysis/ValueTracking.cpp
2209 ↗(On Diff #89562)

It is undef but I don't think ComputeNumSignBits it allowed to assign known bits to undef values.

sanjoy updated this revision to Diff 89594.Feb 23 2017, 5:19 PM
  • Address review
lib/Analysis/ValueTracking.cpp
2209 ↗(On Diff #89562)

I see what you mean. I'm a bit embarrassed now that it took me this long. :)

majnemer added inline comments.Feb 23 2017, 5:43 PM
lib/Analysis/ValueTracking.cpp
2210 ↗(On Diff #89594)

Should it be == or >= ?

sanjoy added inline comments.Feb 23 2017, 5:47 PM
lib/Analysis/ValueTracking.cpp
2210 ↗(On Diff #89594)

The getLimitedValue would've taken care about the > case. Though -- I can change it to getZExtValue() if you want.

majnemer accepted this revision.Feb 24 2017, 2:29 AM

LGTM

lib/Analysis/ValueTracking.cpp
2210 ↗(On Diff #89594)

Ah, I see. I think it'd be slightly more clear if we used getZExtValue here.

This revision is now accepted and ready to land.Feb 24 2017, 2:29 AM
spatel added inline comments.Feb 24 2017, 6:42 AM
lib/Analysis/ValueTracking.cpp
2209 ↗(On Diff #89562)

Since it's non-obvious how undef is handled, how about adding a comment to explain why we don't saturate to TyBits? This could be a function-level comment because this can occur with more than just Ashr (and Shl?).

test/Transforms/IndVarSimplify/pr32045.ll
7 ↗(On Diff #89594)

I think IndVars does make a transformation on this test, so it would be better to check the correct output rather than just checking that no assert occurs. The script at utils/update_test_checks.py can be used with loops now. :)

sanjoy updated this revision to Diff 89763.Feb 24 2017, 6:18 PM
  • Address review
spatel accepted this revision.Feb 25 2017, 8:49 AM

LGTM.

This revision was automatically updated to reflect the committed changes.