Add POP instruction to ARM integrated assembler.

Add POP instruction to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2015-11-09 20:49:02]

Description was changed from

==========
Add POP instruction to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add POP instruction to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-11-09 20:49:02]

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

[John, 2015-11-09 21:43:03]

lgtm

[Jim Stichnoth, 2015-11-09 22:17:26]

https://codereview.chromium.org/1433743002/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:919: constexpr bool isLoad = true;
IsLoad, IsByte

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

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1123: for (SizeT i = 0; i < Dests.size(); ++i) {
Consider using a range-based for loop:

  Variable *LastDest = nullptr; // replaces DestReg
  for (Variable *Var : Dests) {
    ...
    LastDest = Var; // replaces DestReg=i;
  }

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1130: ARM32::IValueT Reg = Var->getRegNum();
I think you're better off declaring this as int32_t.

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1141: if (auto *Var =
llvm::dyn_cast<Variable>(Dests[DestReg])) {
Dests[] is a VarList, so the dyn_cast is unnecessary.

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1157: Asm->popList(GPURegisters, CondARM32::AL);
GPURegisters?  Should this be GPRegisters?

[Karl, 2015-11-09 22:44:35]

https://codereview.chromium.org/1433743002/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:919: constexpr bool isLoad = true;
On 2015/11/09 22:17:26, stichnot wrote:
> IsLoad, IsByte

Done.

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

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1130: ARM32::IValueT Reg = Var->getRegNum();
On 2015/11/09 22:17:26, stichnot wrote:
> I think you're better off declaring this as int32_t.

Forgot that Encoded_Not_GPR is negative. Changing.

https://codereview.chromium.org/1433743002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1141: if (auto *Var =
llvm::dyn_cast<Variable>(Dests[DestReg])) {
On 2015/11/09 22:17:26, stichnot wrote:
> Dests[] is a VarList, so the dyn_cast is unnecessary.

Done.

[Jim Stichnoth, 2015-11-09 23:04:11]

I had something slightly different in mind for the range-based for loops. 
Specifically, the loop is less elegant when you have to have an extra induction
variable - "Index" in this case.

Also, your GPURegister change in the push code brought that to my attention, so
feel free to make corresponding loop changes there. :)

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1120: SizeT DestReg = 0;
remove this

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1123: SizeT Index = 0;
replace this with:
Variable *LastDest = nullptr;

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1131: DestReg = Index;
replace this with:
LastDest = Var;

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1134: ++Index;
remove this

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1141: if (const Variable *Var = Dests[DestReg]) {
use LastDest instead of Dests[DestReg]

Also, LastDest is basically guaranteed to be non-null (though an assert wouldn't
hurt anything), so this whole "if" statement is unneeded.

[Karl, 2015-11-10 16:32:22]

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1120: SizeT DestReg = 0;
On 2015/11/09 23:04:11, stichnot wrote:
> remove this

Done.

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1123: SizeT Index = 0;
On 2015/11/09 23:04:11, stichnot wrote:
> replace this with:
> Variable *LastDest = nullptr;

Done.

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1131: DestReg = Index;
On 2015/11/09 23:04:11, stichnot wrote:
> replace this with:
> LastDest = Var;

Done.

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1134: ++Index;
On 2015/11/09 23:04:11, stichnot wrote:
> remove this

Done.

https://codereview.chromium.org/1433743002/diff/40001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1141: if (const Variable *Var = Dests[DestReg]) {
On 2015/11/09 23:04:11, stichnot wrote:
> use LastDest instead of Dests[DestReg]
> 
> Also, LastDest is basically guaranteed to be non-null (though an assert
wouldn't
> hurt anything), so this whole "if" statement is unneeded.

Done.

https://codereview.chromium.org/1433743002/diff/60001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/60001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1242: for (SizeT Index = 0; Index < getSrcSize(); ++Index)
{
Note: Index is required here because Srcs is an Operand** (i.e. array), and the
length is defined by getSrcSize(). Otherwise, I did changes similar to the pop
instruction.

[Jim Stichnoth, 2015-11-10 16:58:59]

lgtm

https://codereview.chromium.org/1433743002/diff/60001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1433743002/diff/60001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1242: for (SizeT Index = 0; Index < getSrcSize(); ++Index)
{
On 2015/11/10 16:32:22, Karl wrote:
> Note: Index is required here because Srcs is an Operand** (i.e. array), and
the
> length is defined by getSrcSize(). Otherwise, I did changes similar to the pop
> instruction.

Acknowledged.

[Karl, 2015-11-10 22:12:39]

Committed patchset #5 (id:80001) manually as
f66a85b205fc2985c130eebd95a03999394dad0e (presubmit successful).