Subzero. ARM32. Show FP lowering some love.

Subzero. ARM32. Show FP lowering some love.

After some time of being neglected, this CL improves FP lowering for
ARM32.

1) It emits vpush {list}, and vpop {list} when possible.

2) It stops saving alised Vfp registers multiple times (yes, sz used to
save both D and S registers even when they aliased.)

3) Introduces Vmla (fp multiply and accumulate) and Vmls (multiply and
subtract.)

(1 + 2) minimally (but positively) affected SPEC.

(3) caused a 2% geomean improvement.

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

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

[John, 2015-11-27 18:41:29]

Description was changed from

==========
Subzero. ARM32. Show FP lowering some love.

After some time of being neglected, this CL improves FP lowering for
ARM32.

1) It emits vpush {list}, and vpop {list} when possible.

2) It stops saving alised Vfp registers multiple times (yes, sz used to
save both D and S registers even when they aliased.)

3) Introduces Vmla (fp multiply and accumulate) and Vmls (multiply and
subtract.)

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Show FP lowering some love.

After some time of being neglected, this CL improves FP lowering for
ARM32.

1) It emits vpush {list}, and vpop {list} when possible.

2) It stops saving alised Vfp registers multiple times (yes, sz used to
save both D and S registers even when they aliased.)

3) Introduces Vmla (fp multiply and accumulate) and Vmls (multiply and
subtract.)

(1 + 2) minimally (but positively) affected SPEC.

(3) caused a 2% geomean improvement.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

[John, 2015-12-07 15:26:26]

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

[Karl, 2015-12-07 19:11:07]

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1253: return;
I assume you return here because you modified the target lowering to not combine
GP registers (pop) with other registers (vpop)?

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:718: /// by the floating-point (vfp) extension". they do not
set flags.
Why are "We" and "They" switched to "we" and "they". Shouldn't they remain
capitalized, since they begin a sentence.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:809: /// not also provided by the floating-point (vfp)
extension". they do not set
Same here for "they" (last sentence).

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/mov-const.ll (right):

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/mov-const.ll:163: ; IASM-NEXT:	.byte 0x1
Remove tabs.

[Jim Stichnoth, 2015-12-07 20:58:15]

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:582: Type PreviousTy = IceType_void;
What do you think about adding this?

  if (BuildDefs::minimal())
    return;

Also, I think the validation for Pop and Push could be factored out into a
separate function in an anonymous namespace, and I think it's OK if the result
is that you ultimately iterate twice over Srcs in the Push ctor.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1222: bool AssignedConsecutiveRegisters(Operand *Before,
Operand *After) {
Maybe isAssignedConsecutiveRegisters() ?

(in any case, don't capitalize a function name, sorry)

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1223: return llvm::cast<Variable>(Before)->getRegNum() + 1
==
I think you should make Before and After both Variable* type, and do the cast as
necessary in the caller.

Also, shouldn't you also validate that hasReg() is true as part of the test?

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1233: Ostream &Str = Func->getContext()->getStrEmit();
Maybe move this after the DestSize assert / early return

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1235: assert(DestSize > 0);
Maybe move the assert into the "if" branch, right before the return?  I think
that documents the intention slightly better.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1240: Operand *Op = Dests[0];
Can this be declared as Variable* ?  Maybe "Variable *Var"?

In any case, I would name it "Opnd" rather than "Op", because the latter is (in
my opinion) ambiguous between "operand" and "operation" and "opcode".

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:715: /// Instructions of the form x := y op z, for vector/FP.
we leave these as
I think you accidentally lower-cased most of this comment.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:807: /// we leave these as unconditional: "arm deprecates the
conditional execution
Capitalization, as above.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:837: dumpDest(Func);
Doesn't this print Dest too many times, since it's already replicated as the
first Src arg?

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1062: /// regs, but not both. in any case, the list must be
sorted.
In (capitalized)

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1083: /// Pushs a list of registers. Just like Pop (see
above) the list may be of
Pushes

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1083: /// Pushs a list of registers. Just like Pop (see
above) the list may be of
Nit: I would put a comma after "(see above)".

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1184: // callee-saved float/vector registers.
At first glance, this looks like a reflow error.  Maybe add a blank "//" line to
separate paragraphs?

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1204: // Perform a two-pass over the used
registers. The first pass records all the
"Make two passes over the used registers."

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5553: } else if
(OperandARM32FlexImm::canHoldImm(~Value, &RotateAmt,
No "else if" after unconditional return.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1144: using ComputationMap =
std::unordered_map<SizeT, ComputationEntry>;
Add a comment indicating the meaning of the SizeT key.  (I think it's the Dest
variable number, but I no longer remember for sure.)

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
File src/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:1: class RegAliases(object):
This file should probably go into the pydir/ directory instead of src/ .

Also, 80-col.  Sorry.

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:16: IsInt=0, IsI64Pair=0, IsFP32=0, IsFP64=0,
IsVec128=0,
Indent these 2 lines 1 extra space, to line up with the first arg above?

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:19: assert not ((IsFP32 and IsFP64) or (IsFP32 and
IsVec128) or
Can you break this up into 3 individual asserts?

assert not (IsFP32 and IsFP64)
assert not (IsFP32 and IsVec128)
assert not (IsFP64 and IsVec128)

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:32: for Name, Value in self.Features:
I'm not a python expert by any means, but I don't think you would be iterating
through dictionaries or whatever like it's C++ code.

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/mov-const.ll (right):

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/mov-const.ll:144: ; ASM-NEXT:     # [sp, #4084] =
def.pseudo
Trailing whitespace on this line

[John, 2015-12-08 13:54:25]

ptal

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:582: Type PreviousTy = IceType_void;
On 2015/12/07 20:58:14, stichnot wrote:
> What do you think about adding this?
> 
>   if (BuildDefs::minimal())
>     return;
> 
> Also, I think the validation for Pop and Push could be factored out into a
> separate function in an anonymous namespace, and I think it's OK if the result
> is that you ultimately iterate twice over Srcs in the Push ctor.

This seems like a serious error that should be verified regardless of minimal
build, but I could be convinced otherwise. What are the arguments for removing
it?

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1222: bool AssignedConsecutiveRegisters(Operand *Before,
Operand *After) {
On 2015/12/07 20:58:14, stichnot wrote:
> Maybe isAssignedConsecutiveRegisters() ?
> 
> (in any case, don't capitalize a function name, sorry)

Done.

PS: don't be sorry for the code style. :)

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1223: return llvm::cast<Variable>(Before)->getRegNum() + 1
==
On 2015/12/07 20:58:14, stichnot wrote:
> I think you should make Before and After both Variable* type, and do the cast
as
> necessary in the caller.
> 
> Also, shouldn't you also validate that hasReg() is true as part of the test?

I am asserting here because I moved the actual test to the ctor.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1233: Ostream &Str = Func->getContext()->getStrEmit();
On 2015/12/07 20:58:14, stichnot wrote:
> Maybe move this after the DestSize assert / early return

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1235: assert(DestSize > 0);
On 2015/12/07 20:58:14, stichnot wrote:
> Maybe move the assert into the "if" branch, right before the return?  I think
> that documents the intention slightly better.

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1240: Operand *Op = Dests[0];
On 2015/12/07 20:58:14, stichnot wrote:
> Can this be declared as Variable* ?  Maybe "Variable *Var"?
> 
> In any case, I would name it "Opnd" rather than "Op", because the latter is
(in
> my opinion) ambiguous between "operand" and "operation" and "opcode".

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1253: return;
On 2015/12/07 19:11:07, Karl wrote:
> I assume you return here because you modified the target lowering to not
combine
> GP registers (pop) with other registers (vpop)?

That's correct.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:715: /// Instructions of the form x := y op z, for vector/FP.
we leave these as
On 2015/12/07 20:58:14, stichnot wrote:
> I think you accidentally lower-cased most of this comment.

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:718: /// by the floating-point (vfp) extension". they do not
set flags.
On 2015/12/07 19:11:07, Karl wrote:
> Why are "We" and "They" switched to "we" and "they". Shouldn't they remain
> capitalized, since they begin a sentence.

I honestly do not know what happened here... o.O

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:807: /// we leave these as unconditional: "arm deprecates the
conditional execution
On 2015/12/07 20:58:14, stichnot wrote:
> Capitalization, as above.

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:809: /// not also provided by the floating-point (vfp)
extension". they do not set
On 2015/12/07 19:11:07, Karl wrote:
> Same here for "they" (last sentence).

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:837: dumpDest(Func);
On 2015/12/07 20:58:14, stichnot wrote:
> Doesn't this print Dest too many times, since it's already replicated as the
> first Src arg?

These instructions' semantics are

x = x [+/-] (a * b)

so I wanted the dump to be

vml[sa].f?? x, x, a, b

This is also consistent with the integer mla/mls instructions (although those
instructions are actual 4 address instructions.)

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1062: /// regs, but not both. in any case, the list must be
sorted.
On 2015/12/07 20:58:14, stichnot wrote:
> In (capitalized)

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1083: /// Pushs a list of registers. Just like Pop (see
above) the list may be of
On 2015/12/07 20:58:14, stichnot wrote:
> Pushes

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:1083: /// Pushs a list of registers. Just like Pop (see
above) the list may be of
On 2015/12/07 20:58:14, stichnot wrote:
> Nit: I would put a comma after "(see above)".

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1184: // callee-saved float/vector registers.
On 2015/12/07 20:58:14, stichnot wrote:
> At first glance, this looks like a reflow error.  Maybe add a blank "//" line
to
> separate paragraphs?

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1204: // Perform a two-pass over the used
registers. The first pass records all the
On 2015/12/07 20:58:14, stichnot wrote:
> "Make two passes over the used registers."

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5553: } else if
(OperandARM32FlexImm::canHoldImm(~Value, &RotateAmt,
On 2015/12/07 20:58:14, stichnot wrote:
> No "else if" after unconditional return.

In my defense, this was here prior to this CL. :P

I could change this now, on this CL, but that would mean this code would be
inconsistent with the rest of the method. I left a TODO(jpp) on top of the
method as a reminder that I need to refactor legalize. Thoughts?

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1144: using ComputationMap =
std::unordered_map<SizeT, ComputationEntry>;
On 2015/12/07 20:58:14, stichnot wrote:
> Add a comment indicating the meaning of the SizeT key.  (I think it's the Dest
> variable number, but I no longer remember for sure.)

Done -- maybe add a comment to X86 as well? Happy to add it on this CL if you
want.

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
File src/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:1: class RegAliases(object):
On 2015/12/07 20:58:15, stichnot wrote:
> This file should probably go into the pydir/ directory instead of src/ .
> 
> Also, 80-col.  Sorry.

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:16: IsInt=0, IsI64Pair=0, IsFP32=0, IsFP64=0,
IsVec128=0,
On 2015/12/07 20:58:15, stichnot wrote:
> Indent these 2 lines 1 extra space, to line up with the first arg above?

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:19: assert not ((IsFP32 and IsFP64) or (IsFP32 and
IsVec128) or
On 2015/12/07 20:58:14, stichnot wrote:
> Can you break this up into 3 individual asserts?
> 
> assert not (IsFP32 and IsFP64)
> assert not (IsFP32 and IsVec128)
> assert not (IsFP64 and IsVec128)

But I like long, confusing conditions... :P

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/gen_arm32_reg_tabl...
src/gen_arm32_reg_tables.py:32: for Name, Value in self.Features:
On 2015/12/07 20:58:15, stichnot wrote:
> I'm not a python expert by any means, but I don't think you would be iterating
> through dictionaries or whatever like it's C++ code.

Features is not a dictionary -- it is a vector of tuples. I can't use a
dictionary here because that would lead to unpredictable output ordering.

I thought this was confusing before, but I decided that **maybe** it was OK to
keep it this way. I modified the code so now I have an array with the keys in
the appropriate order, and a dictionary, for fast lookups.

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/mov-const.ll (right):

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/mov-const.ll:144: ; ASM-NEXT:     # [sp, #4084] =
def.pseudo
On 2015/12/07 20:58:15, stichnot wrote:
> Trailing whitespace on this line

Done.

https://codereview.chromium.org/1481133002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/mov-const.ll:163: ; IASM-NEXT:	.byte 0x1
On 2015/12/07 19:11:07, Karl wrote:
> Remove tabs.

Done.

[Jim Stichnoth, 2015-12-08 19:20:16]

otherwise lgtm

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:582: Type PreviousTy = IceType_void;
On 2015/12/08 13:54:24, John wrote:
> On 2015/12/07 20:58:14, stichnot wrote:
> > What do you think about adding this?
> > 
> >   if (BuildDefs::minimal())
> >     return;
> > 
> > Also, I think the validation for Pop and Push could be factored out into a
> > separate function in an anonymous namespace, and I think it's OK if the
result
> > is that you ultimately iterate twice over Srcs in the Push ctor.
> 
> This seems like a serious error that should be verified regardless of minimal
> build, but I could be convinced otherwise. What are the arguments for removing
> it?

The only argument for removing it would be a very minor performance argument in
MINIMAL mode, plus one fewer string in the binary.

I'm OK either way.

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1235: assert(DestSize > 0);
On 2015/12/08 13:54:24, John wrote:
> On 2015/12/07 20:58:14, stichnot wrote:
> > Maybe move the assert into the "if" branch, right before the return?  I
think
> > that documents the intention slightly better.
> 
> Done.

Actually, looks like not (yet) done...

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.h#new...
src/IceInstARM32.h:837: dumpDest(Func);
On 2015/12/08 13:54:24, John wrote:
> On 2015/12/07 20:58:14, stichnot wrote:
> > Doesn't this print Dest too many times, since it's already replicated as the
> > first Src arg?
> 
> These instructions' semantics are
> 
> x = x [+/-] (a * b)
> 
> so I wanted the dump to be
> 
> vml[sa].f?? x, x, a, b
> 
> This is also consistent with the integer mla/mls instructions (although those
> instructions are actual 4 address instructions.)

OK, I see.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1144: using ComputationMap =
std::unordered_map<SizeT, ComputationEntry>;
On 2015/12/08 13:54:25, John wrote:
> On 2015/12/07 20:58:14, stichnot wrote:
> > Add a comment indicating the meaning of the SizeT key.  (I think it's the
Dest
> > variable number, but I no longer remember for sure.)
> 
> Done -- maybe add a comment to X86 as well? Happy to add it on this CL if you
> want.

That would be great, thanks.

https://codereview.chromium.org/1481133002/diff/120001/pydir/gen_arm32_reg_ta...
File pydir/gen_arm32_reg_tables.py (right):

https://codereview.chromium.org/1481133002/diff/120001/pydir/gen_arm32_reg_ta...
pydir/gen_arm32_reg_tables.py:61: # Note: The following tables break the usual
80-col on purpose -- it is easier
I get the >80-col readability issue.

Another approach might be to abbreviate the attribute (or whatever it's called)
names so that at least most of the lines fit in 80 columns.  The abbreviations
could be documented both here and in the RegFeatures definition for ultimate
clarity.

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:584: //
Fill in the comment, or delete this line.

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:585: void ValidatePushOrPopRegisterListOrDie(const VarList
&RegList) {
lowerCase function name

https://codereview.chromium.org/1481133002/diff/120001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1159: // Make a two-pass over the used registers.
The first pass records all the
"Make two passes over"

I'm not familiar with the "two-pass". :)

[John, 2015-12-09 13:11:02]

Description was changed from

==========
Subzero. ARM32. Show FP lowering some love.

After some time of being neglected, this CL improves FP lowering for
ARM32.

1) It emits vpush {list}, and vpop {list} when possible.

2) It stops saving alised Vfp registers multiple times (yes, sz used to
save both D and S registers even when they aliased.)

3) Introduces Vmla (fp multiply and accumulate) and Vmls (multiply and
subtract.)

(1 + 2) minimally (but positively) affected SPEC.

(3) caused a 2% geomean improvement.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Show FP lowering some love.

After some time of being neglected, this CL improves FP lowering for
ARM32.

1) It emits vpush {list}, and vpop {list} when possible.

2) It stops saving alised Vfp registers multiple times (yes, sz used to
save both D and S registers even when they aliased.)

3) Introduces Vmla (fp multiply and accumulate) and Vmls (multiply and
subtract.)

(1 + 2) minimally (but positively) affected SPEC.

(3) caused a 2% geomean improvement.

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

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

[John, 2015-12-09 13:11:03]

Committed patchset #8 (id:140001) manually as
eb13acc6221f60642d17473b6329a82b02ae557a (presubmit successful).

[John, 2015-12-09 13:11:09]

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1235: assert(DestSize > 0);
On 2015/12/08 19:20:16, stichnot wrote:
> On 2015/12/08 13:54:24, John wrote:
> > On 2015/12/07 20:58:14, stichnot wrote:
> > > Maybe move the assert into the "if" branch, right before the return?  I
> think
> > > that documents the intention slightly better.
> > 
> > Done.
> 
> Actually, looks like not (yet) done...

Done.

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1481133002/diff/100001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:1144: using ComputationMap =
std::unordered_map<SizeT, ComputationEntry>;
On 2015/12/08 19:20:16, stichnot wrote:
> On 2015/12/08 13:54:25, John wrote:
> > On 2015/12/07 20:58:14, stichnot wrote:
> > > Add a comment indicating the meaning of the SizeT key.  (I think it's the
> Dest
> > > variable number, but I no longer remember for sure.)
> > 
> > Done -- maybe add a comment to X86 as well? Happy to add it on this CL if
you
> > want.
> 
> That would be great, thanks.

The comment was already there. :)

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:584: //
On 2015/12/08 19:20:16, stichnot wrote:
> Fill in the comment, or delete this line.

Done.

https://codereview.chromium.org/1481133002/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:585: void ValidatePushOrPopRegisterListOrDie(const VarList
&RegList) {
On 2015/12/08 19:20:16, stichnot wrote:
> lowerCase function name

Done.

https://codereview.chromium.org/1481133002/diff/120001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1481133002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1159: // Make a two-pass over the used registers.
The first pass records all the
On 2015/12/08 19:20:16, stichnot wrote:
> "Make two passes over"
> 
> I'm not familiar with the "two-pass". :)

Done.