Subzero. ARM32. Address mode formation.

Subzero. ARM32. Address mode formation.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=kschimpf@google.com, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f5f02f7ee856727e2c6dd0063b19932efc4e47f6

[John, 2015-11-05 20:35:41]

jpp@chromium.org changed reviewers:
+ kschimpf@google.com, sehr@chromium.org, stichnot@chromium.org

[Karl, 2015-11-06 21:11:01]

Otherwise LGTM.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:974: int32_t *NewBaseOffset) {
Why not int32_t &NewBaseOffset?

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1176: const bool ZeroExt = false;
constexpr?

[Jim Stichnoth, 2015-11-08 20:05:51]

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1034: assert(Mem->getBase()->getRegNum() ==
(int)getFrameOrStackReg());
int32_t for consistency

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3382: const Variable *OffsetReg, int16_t
OffsetRegShamt,
tiny nit - OffsetRegShAmt ?
(here and below)

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3418: assert(SrcOp);
This assert is a relic of the early days when maybe it was possible for getSrc()
to return nullptr, or maybe the infrastructure wasn't as solid, who knows. 
These days it's just not possible so I would just remove the assert.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3424: *Var = SrcVar;
this statement is unreachable

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3425: } else if (auto *Const =
llvm::dyn_cast<ConstantInteger32>(SrcOp)) {
Don't use "else if" if the previous branch is really supposed to always return.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3427: return false;
Why not return true?
:)

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3444: if (auto *Arith =
llvm::dyn_cast_or_null<const InstArithmetic>(Inst)) {
Can you just use dyn_cast?  It looks like Inst can never be nullptr.

Also, I like your earlier idea of using Instr instead of Inst for the variable
name.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3693: OstreamLocker L(Func->getContext());
Earlier you used "OstreamLocker _(...);"

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3800: static constexpr Type PointerType =
IceType_i32;
Can you use the existing getPointerType() instead?

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:4165: assert(RegBase->hasReg() &&
Two separate asserts for better precision when it fails.

[John, 2015-11-09 22:42:25]

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:974: int32_t *NewBaseOffset) {
On 2015/11/06 21:11:01, Karl wrote:
> Why not int32_t &NewBaseOffset?

Because the method changes NewBaseOffset inside.

I really dislike non-const references as parameters. They make the code harder
to read -- especially if you're not familiar with it. Several times over I
caught myself trying to find how a variable was being initialized, just to find
out a method was taking that parameter by reference and initializing it.

with this signature, at call site, we have

legalizeStackSlot(..., &NewBaseOffset);

so I at least have an idea that legalizeStackSlot may change that value.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1034: assert(Mem->getBase()->getRegNum() ==
(int)getFrameOrStackReg());
On 2015/11/08 20:05:51, stichnot wrote:
> int32_t for consistency

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1176: const bool ZeroExt = false;
On 2015/11/06 21:11:01, Karl wrote:
> constexpr?

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3382: const Variable *OffsetReg, int16_t
OffsetRegShamt,
On 2015/11/08 20:05:51, stichnot wrote:
> tiny nit - OffsetRegShAmt ?
> (here and below)

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3418: assert(SrcOp);
On 2015/11/08 20:05:50, stichnot wrote:
> This assert is a relic of the early days when maybe it was possible for
getSrc()
> to return nullptr, or maybe the infrastructure wasn't as solid, who knows. 
> These days it's just not possible so I would just remove the assert.

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3424: *Var = SrcVar;
On 2015/11/08 20:05:51, stichnot wrote:
> this statement is unreachable

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3425: } else if (auto *Const =
llvm::dyn_cast<ConstantInteger32>(SrcOp)) {
On 2015/11/08 20:05:51, stichnot wrote:
> Don't use "else if" if the previous branch is really supposed to always
return.

N/A.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3427: return false;
On 2015/11/08 20:05:51, stichnot wrote:
> Why not return true?
> :)

I really don't know how high I was here.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3444: if (auto *Arith =
llvm::dyn_cast_or_null<const InstArithmetic>(Inst)) {
On 2015/11/08 20:05:51, stichnot wrote:
> Can you just use dyn_cast?  It looks like Inst can never be nullptr.
> 
> Also, I like your earlier idea of using Instr instead of Inst for the variable
> name.

Done.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3693: OstreamLocker L(Func->getContext());
On 2015/11/08 20:05:51, stichnot wrote:
> Earlier you used "OstreamLocker _(...);"

Changes everywhere.

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3800: static constexpr Type PointerType =
IceType_i32;
On 2015/11/08 20:05:51, stichnot wrote:
> Can you use the existing getPointerType() instead?

Then it is no longer a constexpr. :(

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:4165: assert(RegBase->hasReg() &&
On 2015/11/08 20:05:51, stichnot wrote:
> Two separate asserts for better precision when it fails.

Done.

[Jim Stichnoth, 2015-11-09 22:47:49]

otherwise lgtm

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3800: static constexpr Type PointerType =
IceType_i32;
On 2015/11/09 22:42:24, John wrote:
> On 2015/11/08 20:05:51, stichnot wrote:
> > Can you use the existing getPointerType() instead?
> 
> Then it is no longer a constexpr. :(

Then why did you subsequently change it to "const"?

[John, 2015-11-09 22:52:30]

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1422753010/diff/60001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3800: static constexpr Type PointerType =
IceType_i32;
On 2015/11/09 22:47:49, stichnot wrote:
> On 2015/11/09 22:42:24, John wrote:
> > On 2015/11/08 20:05:51, stichnot wrote:
> > > Can you use the existing getPointerType() instead?
> > 
> > Then it is no longer a constexpr. :(
> 
> Then why did you subsequently change it to "const"?

Done.

[John, 2015-11-09 22:52:44]

Committed patchset #7 (id:120001) manually as
f5f02f7ee856727e2c6dd0063b19932efc4e47f6 (presubmit successful).