Subzero. ARM32. Pre-lowers calls to ARM32 Helpers.

Subzero. ARM32. Pre-lowers calls to ARM32 Helpers.

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

[Jim Stichnoth, 2015-11-24 22:23:59]

stichnot@chromium.org changed reviewers:
+ stichnot@chromium.org

[Jim Stichnoth, 2015-11-24 22:24:00]

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32.cpp
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.cpp:281: break;
maybe just return here?

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:864: /// assignments. The first few integer type
parameters can use r0-r3,
sentence ends in a comma - is there more that got stripped out?

[John, 2015-11-25 16:39:56]

jpp@chromium.org changed reviewers:
+ sehr@chromium.org

[John, 2015-11-25 16:39:57]

This is now ready for a full-blown review.

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32.cpp
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.cpp:281: break;
On 2015/11/24 22:24:00, stichnot wrote:
> maybe just return here?

Done.

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32.h
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1474883002/diff/1/src/IceTargetLoweringARM32....
src/IceTargetLoweringARM32.h:864: /// assignments. The first few integer type
parameters can use r0-r3,
On 2015/11/24 22:24:00, stichnot wrote:
> sentence ends in a comma - is there more that got stripped out?

No idea what happened here.

[Jim Stichnoth, 2015-11-30 21:47:20]

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:360: NewC &= !(0x80000000 >> ShAmt);
Did you mean "~" instead of "!" ?

Also, what do you think about using int32_t(0x80000000)>>ShAmt to make it more
clear that it's an arithmetic shift of a negative number?

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:401: llvm_unreachable("Control flow should never
have reached here.");
Except for vector instructions...  How about making this report_fatal_error()
just to be safe?

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:919: void Ctpop64Postamble(const InstCall *Instr);
Start function names with a lowercase letter, per LLVM convention.

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:922: ARM32HelpersPreamble;
The prefix "ARM32" seems redundant with this being part of an ARM32 class.

https://codereview.chromium.org/1474883002/diff/120001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsl.ll (right):

https://codereview.chromium.org/1474883002/diff/120001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:23: define internal i32 @_Z7testIs0h(i32 %a) {
Optional: Consider renaming to something that's not a scary C++ mangled name.

[John, 2015-11-30 22:16:25]

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:360: NewC &= !(0x80000000 >> ShAmt);
On 2015/11/30 21:47:20, stichnot wrote:
> Did you mean "~" instead of "!" ?
> 
> Also, what do you think about using int32_t(0x80000000)>>ShAmt to make it more
> clear that it's an arithmetic shift of a negative number?

are you OK with 0x80000000l instead? I don't really like using PODs as if they
were ctors.

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:401: llvm_unreachable("Control flow should never
have reached here.");
On 2015/11/30 21:47:19, stichnot wrote:
> Except for vector instructions...  How about making this report_fatal_error()
> just to be safe?

Done.

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:919: void Ctpop64Postamble(const InstCall *Instr);
On 2015/11/30 21:47:20, stichnot wrote:
> Start function names with a lowercase letter, per LLVM convention.

Done.

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:922: ARM32HelpersPreamble;
On 2015/11/30 21:47:20, stichnot wrote:
> The prefix "ARM32" seems redundant with this being part of an ARM32 class.

I agree. Previously I had HelpersPreamble/HelpersPostamble (without the ARM32),
but that was an eye sore. Those names don't really convey any decent
information. Not that ARM32Helpers* is any better, but I could not think of
anything better.

https://codereview.chromium.org/1474883002/diff/120001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsl.ll (right):

https://codereview.chromium.org/1474883002/diff/120001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:23: define internal i32 @_Z7testIs0h(i32 %a) {
On 2015/11/30 21:47:20, stichnot wrote:
> Optional: Consider renaming to something that's not a scary C++ mangled name.

Done.

[Jim Stichnoth, 2015-12-01 17:01:56]

lgtm

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:360: NewC &= !(0x80000000 >> ShAmt);
On 2015/11/30 22:16:25, John wrote:
> On 2015/11/30 21:47:20, stichnot wrote:
> > Did you mean "~" instead of "!" ?
> > 
> > Also, what do you think about using int32_t(0x80000000)>>ShAmt to make it
more
> > clear that it's an arithmetic shift of a negative number?
> 
> are you OK with 0x80000000l instead? I don't really like using PODs as if they
> were ctors.

OK.  I think at least the presence of a suffix hints at the special intent, so
that's good.

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:401: llvm_unreachable("Control flow should never
have reached here.");
On 2015/11/30 22:16:25, John wrote:
> On 2015/11/30 21:47:19, stichnot wrote:
> > Except for vector instructions...  How about making this
report_fatal_error()
> > just to be safe?
> 
> Done.

Thanks.  I marked a few more cases that I think ought to be report_fatal_error. 
Optionally, make them all into report_fatal_error.

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

https://codereview.chromium.org/1474883002/diff/120001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:922: ARM32HelpersPreamble;
On 2015/11/30 22:16:25, John wrote:
> On 2015/11/30 21:47:20, stichnot wrote:
> > The prefix "ARM32" seems redundant with this being part of an ARM32 class.
> 
> I agree. Previously I had HelpersPreamble/HelpersPostamble (without the
ARM32),
> but that was an eye sore. Those names don't really convey any decent
> information. Not that ARM32Helpers* is any better, but I could not think of
> anything better.

Acknowledged.

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

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1516: llvm_unreachable("Unsupported operand
type");
report_fatal_error

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1576: llvm_unreachable("Unsupported operand
type");
report_fatal_error

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5347: llvm_unreachable("Unhandled operand kind in
legalize()");
report_fatal_error

[John, 2015-12-01 20:56:27]

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

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1516: llvm_unreachable("Unsupported operand
type");
On 2015/12/01 17:01:56, stichnot wrote:
> report_fatal_error

Done.

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1576: llvm_unreachable("Unsupported operand
type");
On 2015/12/01 17:01:56, stichnot wrote:
> report_fatal_error

Done.

https://codereview.chromium.org/1474883002/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:5347: llvm_unreachable("Unhandled operand kind in
legalize()");
On 2015/12/01 17:01:56, stichnot wrote:
> report_fatal_error

Done.

[John, 2015-12-01 21:00:46]

Description was changed from

==========
Subzero. ARM32. Pre-lowers calls to ARM32 Helpers.

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

to

==========
Subzero. ARM32. Pre-lowers calls to ARM32 Helpers.

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-01 21:00:47]

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