Fix ARM assembler to pop registers in reverse order of pushes.

Fix ARM assembler to pop registers in reverse order of pushes.

BUG=None
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-02-04 23:37:24]

Description was changed from

==========
Fix ARM assembler to pop registers in reverse order of pushes.

BUG=None
==========

to

==========
Fix ARM assembler to pop registers in reverse order of pushes.

BUG=None
==========

[Karl, 2016-02-04 23:37:25]

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

[Jim Stichnoth, 2016-02-05 00:48:17]

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1776: return Before->getRegNum() - 1 == After->getRegNum();
I realize this is just transplanted code, but...

This code is relying on a certain ordering of the register enum values as laid
out in the .def file.  I want to be able to change/scramble the order and have
the generated code continue to work (though not necessarily being precisely as
efficient).

I think to be safe here, you should actually be comparing the encoded values,
not the enum values.

[John, 2016-02-05 01:46:25]

lgtm

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1776: return Before->getRegNum() - 1 == After->getRegNum();
On 2016/02/05 00:48:17, stichnot wrote:
> I realize this is just transplanted code, but...
> 
> This code is relying on a certain ordering of the register enum values as laid
> out in the .def file.  I want to be able to change/scramble the order and have
> the generated code continue to work (though not necessarily being precisely as
> efficient).
> 
> I think to be safe here, you should actually be comparing the encoded values,
> not the enum values.

The only issue with encoded reg values (that is also a problem with the current
implementation) is that it will not catch resister type mix ups. But agreed,
using the encoded reg value is better.

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1779: Variable *InstARM32Pop::getStackSReg(SizeT Index,
SizeT Size) const {
Do you need to pass Size here? Isn't size the same as Dest.size()? If so, just
invoke it here

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.h#newcode1144
src/IceInstARM32.h:1144: VarList Dests;
For future reference: you could have marked the class final instead. But please
do not change it. :)

[Karl, 2016-02-05 17:00:05]

To deal with the fact that there is a "cutoff" on the number of consecutive
registers that can appear in a vpush/vpop, I rewrote the code to (a) collect the
sequence of instructions to generate, and then (b) generate the instructions in
the appropriate order. This guarantees we get the same form for the same list of
registers.

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1776: return Before->getRegNum() - 1 == After->getRegNum();
On 2016/02/05 01:46:25, John wrote:
> On 2016/02/05 00:48:17, stichnot wrote:
> > I realize this is just transplanted code, but...
> > 
> > This code is relying on a certain ordering of the register enum values as
laid
> > out in the .def file.  I want to be able to change/scramble the order and
have
> > the generated code continue to work (though not necessarily being precisely
as
> > efficient).
> > 
> > I think to be safe here, you should actually be comparing the encoded
values,
> > not the enum values.
> 
> The only issue with encoded reg values (that is also a problem with the
current
> implementation) is that it will not catch resister type mix ups. But agreed,
> using the encoded reg value is better.

Done.

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.cpp#newcod...
src/IceInstARM32.cpp:1779: Variable *InstARM32Pop::getStackSReg(SizeT Index,
SizeT Size) const {
On 2016/02/05 01:46:25, John wrote:
> Do you need to pass Size here? Isn't size the same as Dest.size()? If so, just
> invoke it here

No longer needed. Removed need for virtuals.

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1669973002/diff/1/src/IceInstARM32.h#newcode1144
src/IceInstARM32.h:1144: VarList Dests;
On 2016/02/05 01:46:25, John wrote:
> For future reference: you could have marked the class final instead. But
please
> do not change it. :)

Acknowledged.

[Jim Stichnoth, 2016-02-08 17:15:58]

lgtm

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:952: llvm::SmallVector<std::pair<const Variable *, SizeT>,
5> InstData;
Could you document the choice of "5"?  With a constexpr and/or comment perhaps
like:

  // Typical max number of registers pushed/popped is no more than 5.

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:965: InstData.push_back(std::make_pair(BaseReg, RegCount));
InstData.emplace_back(BaseReg, RegCount);

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:972: InstData.push_back(std::make_pair(BaseReg, RegCount));
emplace_back

https://codereview.chromium.org/1669973002/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/popmult.ll (right):

https://codereview.chromium.org/1669973002/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/popmult.ll:58: ; ASM-NEXT:     # lr = def.pseudo
trailing whitespace character

[Karl, 2016-02-09 20:23:59]

Description was changed from

==========
Fix ARM assembler to pop registers in reverse order of pushes.

BUG=None
==========

to

==========
Fix ARM assembler to pop registers in reverse order of pushes.

BUG=None
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-02-09 20:24:00]

Committed patchset #3 (id:40001) manually as
00c30380f299239b4782a0fd1ad1d580d12af649 (presubmit successful).

[Karl, 2016-02-09 20:24:27]

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:952: llvm::SmallVector<std::pair<const Variable *, SizeT>,
5> InstData;
On 2016/02/08 17:15:58, stichnot wrote:
> Could you document the choice of "5"?  With a constexpr and/or comment perhaps
> like:
> 
>   // Typical max number of registers pushed/popped is no more than 5.

Done.

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:965: InstData.push_back(std::make_pair(BaseReg, RegCount));
On 2016/02/08 17:15:58, stichnot wrote:
> InstData.emplace_back(BaseReg, RegCount);

Done.

https://codereview.chromium.org/1669973002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:972: InstData.push_back(std::make_pair(BaseReg, RegCount));
On 2016/02/08 17:15:58, stichnot wrote:
> emplace_back

Done.