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.
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.)
|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.
|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.
|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.