Subzero. ARM32. Improve constant lowering.

Subzero. ARM32. Improve constant lowering.

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=ccea793fe4259ba9aa0b6bcd3f281a5c08ac2aa4

[John, 2015-11-12 22:40:28]

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

[sehr, 2015-11-13 21:56:29]

A few comments.  Still working on this.

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:299: static constexpr uint32_t a = 0x80000000;
0x80000000u?

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:304: static_assert(AllowedBits == 0xFFF80000,
Ditto regarding u suffix, here and below wherever the constant is actually
negative?

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1801: // for filetype=asm. Why?
How did you do the double version?

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:224: using OperandARM32::dump;
What does this using do?

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1433: // Technically, ARM has their own aeabi
routines, but we can use the
either "has its" or "have their".

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2705: 
Is there a more common place for this sort of function?  IceOperand.h?

[John, 2015-11-13 22:00:41]

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2705: 
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> Is there a more common place for this sort of function?  IceOperand.h?

Maybe. If you think it's useful, you could add from your cl, and i would use it
here.

[John, 2015-11-14 00:00:38]

I am working on folding constants on every integer instruction, but here are the
responses to your comments.

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:299: static constexpr uint32_t a = 0x80000000;
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> 0x80000000u?

Done.

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:304: static_assert(AllowedBits == 0xFFF80000,
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> Ditto regarding u suffix, here and below wherever the constant is actually
> negative?

Done.

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:1801: // for filetype=asm. Why?
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> How did you do the double version?

almost the same things, except that the constant is of the form

aBbbbbbb bbcdefgh 00000000 00000000 00000000 00000000 00000000 00000000

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:224: using OperandARM32::dump;
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> What does this using do?

Operand::dump is overloaded. The

 OperandARM32FlexFpImm::dump(const Cfg*, Ostream &) below 

defined below hides the two overloads in the base case. Thus, we need to tell
the compiler that those methods are available. (This is why Google3 coding style
forbids overloaded virtual methods)

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1433: // Technically, ARM has their own aeabi
routines, but we can use the
On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> either "has its" or "have their".

For a moment I thought this was Jim. :)

Done.

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2705: 
On 2015/11/13 22:00:41, John wrote:
> On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> > Is there a more common place for this sort of function?  IceOperand.h?
> 
> Maybe. If you think it's useful, you could add from your cl, and i would use
it
> here.

Oh, I thought this was Jim. He had the same routine in the pxor cl. I changed
the implementation to invoke the newly added Utils::isPositiveZero method.

[Jim Stichnoth, 2015-11-16 13:56:10]

lgtm

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

https://codereview.chromium.org/1438773004/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1467: // Src0Hi is not always used got Shl, and
Src0Lo is not always used for Lhsr.
s/got/for/ ?
Lshr

[John, 2015-11-16 18:57:36]

please take another look. quite a lot changed.

[Jim Stichnoth, 2015-11-16 23:06:26]

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1438773004/diff/120001/src/IceInstARM32.h#new...
src/IceInstARM32.h:224: using OperandARM32::dump;
On 2015/11/14 00:00:38, John wrote:
> On 2015/11/13 21:56:29, sehr (please use this account) wrote:
> > What does this using do?
> 
> Operand::dump is overloaded. The
> 
>  OperandARM32FlexFpImm::dump(const Cfg*, Ostream &) below 
> 
> defined below hides the two overloads in the base case. Thus, we need to tell
> the compiler that those methods are available. (This is why Google3 coding
style
> forbids overloaded virtual methods)

Interesting.  Maybe we (I) should later take a pass over dump() and remove its
silly overloads.

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

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1406: // It holds the two operantion's sources,
and maintains some state as to whether
operations'

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1411: // The class is split in a base class with
operand type-independent methods, and
split into ?

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1419: // NumericOperands<ConstantInt32> also
exposes helper methods for emiting
emitting

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1630: _adds(T_Lo, SrcsLo.src0R(this),
SrcsLo.src1RF(this));
Don't do this.  src0R() and src1RF() have side effects whose order is undefined.
 The lowering could be different across different C++ compilers, and this could
affect register allocation.

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1776: Operand *_32 =
legalize(Ctx->getConstantInt32(32), Legal_Reg | Legal_Flex);
Maybe this should be named _32RF?

[Jim Stichnoth, 2015-11-17 04:00:36]

https://codereview.chromium.org/1438773004/diff/180001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1438773004/diff/180001/src/IceUtils.h#newcode126
src/IceUtils.h:126: return Val == 0 && !signbit(Val);
std::signbit

[John, 2015-11-17 12:58:41]

Committed patchset #12 (id:220001) manually as
ccea793fe4259ba9aa0b6bcd3f281a5c08ac2aa4 (presubmit successful).

[John, 2015-11-17 22:17:05]

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

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1406: // It holds the two operantion's sources,
and maintains some state as to whether
On 2015/11/16 23:06:26, stichnot wrote:
> operations'

well, this is

the two sources of the operation

not

the sources of the two operations

This is now "it holds the two source operands."

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1411: // The class is split in a base class with
operand type-independent methods, and
On 2015/11/16 23:06:26, stichnot wrote:
> split into ?

Done.

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1419: // NumericOperands<ConstantInt32> also
exposes helper methods for emiting
On 2015/11/16 23:06:26, stichnot wrote:
> emitting

Done.

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1630: _adds(T_Lo, SrcsLo.src0R(this),
SrcsLo.src1RF(this));
On 2015/11/16 23:06:26, stichnot wrote:
> Don't do this.  src0R() and src1RF() have side effects whose order is
undefined.
>  The lowering could be different across different C++ compilers, and this
could
> affect register allocation.

doh... done.

https://codereview.chromium.org/1438773004/diff/180001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1776: Operand *_32 =
legalize(Ctx->getConstantInt32(32), Legal_Reg | Legal_Flex);
On 2015/11/16 23:06:26, stichnot wrote:
> Maybe this should be named _32RF?

I'd rather not. This is the number 32, not something that might end up in a
register. It is a flex operand because arm does not have immediates*, it has
flexible immediates**

I could rename this _32F, but then I would be confused about this being 32.0.
I'll leave _32.

*except for mov/movt/call
**which are awesome.

https://codereview.chromium.org/1438773004/diff/180001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/1438773004/diff/180001/src/IceUtils.h#newcode126
src/IceUtils.h:126: return Val == 0 && !signbit(Val);
On 2015/11/17 04:00:36, stichnot wrote:
> std::signbit

Done.