Page MenuHomePhabricator

[ValueTracking] Fix PR23011.

Authored by sanjoy on Mar 24 2015, 9:34 PM.



ComputeNumSignBits returns incorrect results for srem instructions.
This change fixes the issue and adds a test case.

Diff Detail


Event Timeline

sanjoy updated this revision to Diff 22626.Mar 24 2015, 9:34 PM
sanjoy retitled this revision from to [ValueTracking] Fix PR23011..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added reviewers: nadav, nicholas, atrick.
sanjoy added a subscriber: Unknown Object (MLST).
nicholas added inline comments.Mar 25 2015, 12:40 AM
1937 ↗(On Diff #22626)

Blank comment line.

1941 ↗(On Diff #22626)

Is this ever different from Denominator.ceilLogBase2()?

1 ↗(On Diff #22626)

File name is off "pr230011.ll" should be "pr23011.ll".

Also, if this is a bug in value tracking, isn't there a more direct way to observe it than this? Maybe just srem + and and then opt -instsimplify?

sanjoy added inline comments.Mar 25 2015, 12:59 AM
1937 ↗(On Diff #22626)

Will fix.

1941 ↗(On Diff #22626)

The only case where this differs from TyBits - ceilLogBase2 is when Denominator = 1 -- TyBits - ceilLogBase2 will correctly be TyBits in that case (i.e. all bits are sign bits) but the current version of the code will return 0 (i.e. no bits are sign bits). I noticed this just now, and it was not intentional. I can think of two ways to fix this:

  • use ceilLogBase2. I had decided to against this because I think it is easier to see why the analysis is correct in the current form (i.e. with CLZ/CLO).
  • special case WhenNegative (or the whole computation) when Denominator is 1.

I'd prefer doing the latter. WDYT?

1 ↗(On Diff #22626)

Yes. I think I'll also switch out the constant-foldable instruction with something like srem %foo, 3.

nadav edited edge metadata.Mar 25 2015, 9:08 AM

Nick, Sanjoy,

I am sorry for the bug. Thanks for catching and fixing it. One easy way to write a testcase is to rely on the 'overflow' optimizations in instcombine. See the function overflow_mod_mul in Transforms/InstCombine/intrinsics.ll.


nlewycky added inline comments.
1941 ↗(On Diff #22626)

I have a weak preference to keep the comment explaining it as it does, then to use ceilLogBase2 in the implementation. I like code reuse and don't think Denominator==1 should ever make it this far so I'd rather not special case it, but either way is just fine with me.

sanjoy updated this revision to Diff 22679.Mar 25 2015, 2:29 PM
sanjoy edited edge metadata.

Address review:

  • use ceilLogBase2
  • fix test case file name
  • test case to use @llvm.smul.with.overflow.i8
nlewycky accepted this revision.Mar 25 2015, 2:51 PM
nlewycky added a reviewer: nlewycky.


This revision is now accepted and ready to land.Mar 25 2015, 2:51 PM
This revision was automatically updated to reflect the committed changes.