This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Merge the multiword code from lshrInPlace and tcShiftRight into a single implementation
ClosedPublic

Authored by craig.topper on Apr 15 2017, 11:31 PM.

Details

Summary

This merges the two different multiword shift right implementations into a single version located in tcShiftRight. lshrInPlace now calls tcShiftRight for the multiword case.

I retained the memmove fast path from lshrInPlace and used a memset for the zeroing. The for loop is basically tcShiftRight's implementation with the zeroing and the intra-shift of 0 removed.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 15 2017, 11:31 PM

Fix diff context

hans accepted this revision.Apr 17 2017, 1:39 PM

lgtm with a naming suggestion

Are there tests covering this? I couldn't find any direct tests, but are there at least indirect ones that would break if for example you change the function to not shift at all?

Do we have the same situation with tcShiftLeft and shlSlowCase? I suppose it's not quite the same since shlSlowCase isn't in-place, but maybe there's some code to be shared anyway?

lib/Support/APInt.cpp
2690 ↗(On Diff #95399)

Hmm, seems it's spelled COUNT like this in many other parts of the file, but I don't think we do that generally in LLVM; I'd prefer Count. Maybe not worth changing now :-/

2697 ↗(On Diff #95399)

Use // instead of /*

2698 ↗(On Diff #95399)

Actually, Jump doesn't seem like a great name. Maybe we could have WordShift and BitShift?

This revision is now accepted and ready to land.Apr 17 2017, 1:39 PM

I want to look at left shift at some point. Also making an in place version of ashr as well. lshrInPlace was just added last week which is why I focused on it.

Update comments and variable names. Add some basic tests.

This revision was automatically updated to reflect the committed changes.