This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix for PR35688 - handle out-of-range values for r+r to r+i conversion
ClosedPublic

Authored by nemanjai on Dec 18 2017, 3:12 PM.

Details

Summary

Revision 320791 introduced a pass that transforms reg+reg instructions to reg+imm if they're fed by "load immediate". However, it didn't handle out-of-range shifts correctly as reported in PR35688. This patch fixes that and therefore the PR.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Dec 18 2017, 3:12 PM
nemanjai updated this revision to Diff 127427.Dec 18 2017, 3:47 PM

The update_llc_checks.py script seems to have produced a CHECK directive that doesn't actually pass. Fixed the test case.

bkramer added inline comments.Dec 19 2017, 5:07 AM
lib/Target/PowerPC/PPCInstrInfo.cpp
2912 ↗(On Diff #127427)

Is this correct? This would turn

li c, 65
srad x, x, c

into

sradi x, x, 1

My PPC asm is a bit rusty, but those two things don't look
like they do the same thing.

nemanjai added inline comments.Dec 19 2017, 8:54 AM
lib/Target/PowerPC/PPCInstrInfo.cpp
2912 ↗(On Diff #127427)

The two would indeed have different semantics. The former would replicate the sign bit in every bit in register x. The latter would shift right by 1 bit and replicate the sign bit in the vacated high-order bit.

However, the patch will not do that transformation. Please note that for algebraic right shifts, we III.TruncateImmTo = 0; which will ensure we just bail on the transformation if the immediate is wider than 6 bits (5 bits for sraw and friends).

nemanjai updated this revision to Diff 128259.Dec 27 2017, 10:38 PM

Fix the UB identified by Ben Kramer - thanks Ben.

A test case for this edge case would be great.

lib/Target/PowerPC/PPCInstrInfo.cpp
2461 ↗(On Diff #128259)

Use LLU instead of LU, otherwise it won't work on 32 bit platforms.

nemanjai updated this revision to Diff 128287.Dec 28 2017, 4:09 AM

Add a test case for the edge case where operand 3 of RLWINM is a zero.

bkramer accepted this revision.Dec 28 2017, 4:34 AM

lgtm

This revision is now accepted and ready to land.Dec 28 2017, 4:34 AM
This revision was automatically updated to reflect the committed changes.