[SubZero] lower float and double constants for MIPS

[SubZero] lower float and double constants for MIPS

The patch emits constant pool for float and double constants.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-09-19 04:47:53]

Description was changed from

==========
[SubZero] lower float and double constants for MIPS

The patch emits constant pool for float and double constants.
==========

to

==========
[SubZero] lower float and double constants for MIPS

The patch emits constant pool for float and double constants.
==========

[jaydeep.patil, 2016-09-19 04:47:53]

jaydeep.patil@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
sagar.thakur@imgtec.com, srdjan.obucina@imgtec.com, stichnot@chromium.org

[Jim Stichnoth, 2016-09-19 20:50:14]

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3463: const char
ConstantPoolEmitterTraits<double>::AsmTag[] = ".word";
I was wondering about the use of ".word" for both types here, while ARM does
.long/.quad.  (Also that ARM's emitConstant() is very clean without needing to
test Traits::IceType.)

It seems that llvm-mc does support .quad.  This asm line:
	.quad   0x123456789abcdef0
assembles/objdumps to:
   0:	9abcdef0 	lwr	gp,-8464(s5)
   4:	12345678 	beq	s1,s4,159e8 <test_nans+0x15938>

I'm not sure about endian-ness here, but looking at the raw .o file, I see
f0debc9a78563412 (where the lower address contains 0xf0 and the higher address
contains 0x12.

So I'm wondering if this needs to diverge from the more elegant ARM32 version?

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (left):

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:39: return
!Utils::isPositiveZero(ConstDouble->getValue());
Question about this change.

Both ARM and x86 have idioms for loading a +0.0 pattern into a floating-point
register without requiring a memory load, usually something involving an xor
instruction.

Does MIPS32 have something similar?

If not, then this change is fine.

[jaydeep.patil, 2016-09-20 07:28:27]

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3463: const char
ConstantPoolEmitterTraits<double>::AsmTag[] = ".word";
On 2016/09/19 20:50:14, stichnot wrote:
> I was wondering about the use of ".word" for both types here, while ARM does
> .long/.quad.  (Also that ARM's emitConstant() is very clean without needing to
> test Traits::IceType.)
> 
> It seems that llvm-mc does support .quad.  This asm line:
> 	.quad   0x123456789abcdef0
> assembles/objdumps to:
>    0:	9abcdef0 	lwr	gp,-8464(s5)
>    4:	12345678 	beq	s1,s4,159e8 <test_nans+0x15938>
> 
> I'm not sure about endian-ness here, but looking at the raw .o file, I see
> f0debc9a78563412 (where the lower address contains 0xf0 and the higher address
> contains 0x12.
> 
> So I'm wondering if this needs to diverge from the more elegant ARM32 version?

Done.

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32.h
File src/IceTargetLoweringMIPS32.h (left):

https://codereview.chromium.org/2351583002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.h:39: return
!Utils::isPositiveZero(ConstDouble->getValue());
On 2016/09/19 20:50:14, stichnot wrote:
> Question about this change.
> 
> Both ARM and x86 have idioms for loading a +0.0 pattern into a floating-point
> register without requiring a memory load, usually something involving an xor
> instruction.
> 
> Does MIPS32 have something similar?
> 
> If not, then this change is fine.

Yes, for float/double constant 0 we can use mtc1 $zero.

[Jim Stichnoth, 2016-09-20 15:37:45]

lgtm

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp_const_pool.ll (right):

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp_const_pool.ll:34: ; MIPS32: lui [[REG:.*]],{{.*}}:
R_MIPS_HI16 .L$float$80000000
This is OK for now, but I want to point out that these label names are not
guaranteed to be stable, i.e. in some future CL we may decide we need to change
the naming convention, and then any tests that rely on specific label names
would have to be changed as well.  Unlikely but possible.

The label names should always start with ".L" though.

It's probably better to match symbolically:
; MIPS32: ... R_MIPS_HI16 [[FLOAT_M0:\.L.*]]
and then use [[FLOAT_M0]] down in the constant pool definitions.

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp_const_pool.ll:118: ; MIPS32CP: .word 0x7fc00000 /*
f32 nan */
Similar comment as above.  The "/* f32 nan */" part of the output is not
necessarily stable, so you could just match like this:
; MIPS32CP: .word 0x7fc00000

[Jim Stichnoth, 2016-09-20 15:38:16]

Description was changed from

==========
[SubZero] lower float and double constants for MIPS

The patch emits constant pool for float and double constants.
==========

to

==========
[SubZero] lower float and double constants for MIPS

The patch emits constant pool for float and double constants.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-20 15:38:18]

Committed patchset #2 (id:20001) manually as
cf9c12f84aa58a3025d753c8bab1a4492bd37039 (presubmit successful).

[jaydeep.patil, 2016-09-21 04:52:33]

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fp_const_pool.ll (right):

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp_const_pool.ll:34: ; MIPS32: lui [[REG:.*]],{{.*}}:
R_MIPS_HI16 .L$float$80000000
On 2016/09/20 15:37:45, stichnot wrote:
> This is OK for now, but I want to point out that these label names are not
> guaranteed to be stable, i.e. in some future CL we may decide we need to
change
> the naming convention, and then any tests that rely on specific label names
> would have to be changed as well.  Unlikely but possible.
> 
> The label names should always start with ".L" though.
> 
> It's probably better to match symbolically:
> ; MIPS32: ... R_MIPS_HI16 [[FLOAT_M0:\.L.*]]
> and then use [[FLOAT_M0]] down in the constant pool definitions.

Acknowledged.

https://codereview.chromium.org/2351583002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fp_const_pool.ll:118: ; MIPS32CP: .word 0x7fc00000 /*
f32 nan */
On 2016/09/20 15:37:45, stichnot wrote:
> Similar comment as above.  The "/* f32 nan */" part of the output is not
> necessarily stable, so you could just match like this:
> ; MIPS32CP: .word 0x7fc00000

Acknowledged.