Page MenuHomePhabricator

Consider isLegalAddressingImm in Constant Hoisting
ClosedPublic

Authored by zzheng on Aug 17 2018, 4:05 PM.

Details

Summary
In Thumb1, legal imm range is [0, 255] for ADD/SUB instructions. However, the
legal imm range for LD/ST in (R+Imm) addressing mode is [0, 127]. Imms in
[128, 255] are materialized by mov R, #imm, and LD/STs use them in (R+R)
addressing mode.

This patch checks if a constant is used as offset in (R+Imm), if so, it checks
isLegalAddressingMode passing the constant value as BaseOffset.

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng created this revision.Aug 17 2018, 4:05 PM
zzheng edited reviewers, added: srhines, ributzka; removed: zinob, yinma.Aug 17 2018, 4:11 PM

Please upload patches with full context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

This could probably use a few more tests to show the correct type is getting passed to isLegalAddressingMode (e.g. on Thumb1, an i8 load only supports offsets 0-31, but an i32 load supports offsets 0-124.)

zzheng updated this revision to Diff 161836.Aug 21 2018, 3:56 PM

Updated test case as Eli suggested.

efriedma added inline comments.Aug 23 2018, 3:10 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
594 ↗(On Diff #161836)

Oh, oops, should have spotted the issue here sooner.

This is checking that the use is a store instruction, but it isn't checking that the value is being used as an address. So it'll do the wrong thing for an instruction like store i32* inttoptr (i32 805874688 to i16*), i32** %foo.

608 ↗(On Diff #161836)

I think HasBaseReg should be true; the base is the hoisted constant. Not sure that actually makes a difference in practice; the ARM backend, at least, doesn't check HasBaseReg when the Scale is zero.

zzheng updated this revision to Diff 162514.Aug 24 2018, 5:14 PM

Updated as Eli suggested.

zzheng marked 2 inline comments as done.Aug 24 2018, 5:15 PM
zzheng added inline comments.
lib/Transforms/Scalar/ConstantHoisting.cpp
594 ↗(On Diff #161836)

Nice catch.

zzheng updated this revision to Diff 162767.Aug 27 2018, 4:06 PM
zzheng marked an inline comment as done.

Previous version mistakenly checks MinValItr->ConstInst rather than CC->ConstInst against StoreInst User's pointer operand.

efriedma added inline comments.Aug 27 2018, 4:12 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
598 ↗(On Diff #162767)

The break should be inside the if statement, I think?

I guess more broadly, it's not clear what we should do if there are mixed address and non-address uses... but the current behavior is reasonable.

zzheng updated this revision to Diff 162781.Aug 27 2018, 4:55 PM

Moved 'break;' under proper if statement.

zzheng marked an inline comment as done.Aug 27 2018, 4:56 PM
efriedma added inline comments.Aug 27 2018, 6:39 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
596 ↗(On Diff #162781)

Looking again, this doesn't work. ConstInt is a ConstantInt, SI->getPointerOperand() is a pointer.

ConstantUser has a member OpndIdx which might be helpful here. And more test coverage for stores would be nice.

zzheng updated this revision to Diff 162972.Aug 28 2018, 3:46 PM
zzheng marked an inline comment as done.
This revision is now accepted and ready to land.Aug 28 2018, 3:49 PM
This revision was automatically updated to reflect the committed changes.