Subzero. Implements TargetDataARM32::lowerConstants.

Subzero. Implements TargetDataARM32::lowerConstants.

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

[John, 2015-09-17 21:15:26]

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

[Jim Stichnoth, 2015-09-18 13:59:34]

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3156: static_assert(sizeof(unsigned long long) ==
8,
Can you use uint64_t instead of unsigned long long?

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3203: Str << "\t@" << Traits::TypeName << " " <<
Value << "\n";
Optional: It would be cool if, as an end-of-line comment, you could also print
the non-hex FP representation of the constant.

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3213: std::max(static_cast<size_t>(4),
typeAlignInBytes(Traits::IceType));
First, I don't like magic constants like "4" sprinkled in the code.

Second, why make 4 the minimum alignment?  Can't an i16 constant pool be aligned
at 2-byte boundaries?

[John, 2015-09-18 14:20:25]

A non-trivial merge waits me :( (https://codereview.chromium.org/1356763004/)

On the bright side, we will have lowerFcmp and lowerSelect(IceType_f??) ready by
EOD today. :)

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3156: static_assert(sizeof(unsigned long long) ==
8,
On 2015/09/18 13:59:34, stichnot wrote:
> Can you use uint64_t instead of unsigned long long?

Sometimes you might run into problems with uint64_t. This is not one of those,
some i don't really care either way. Done.

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3203: Str << "\t@" << Traits::TypeName << " " <<
Value << "\n";
On 2015/09/18 13:59:34, stichnot wrote:
> Optional: It would be cool if, as an end-of-line comment, you could also print
> the non-hex FP representation of the constant.

I am already printing the actual FP value (e.g., 1.0). Do you mean outputting
something like:

.quad 0x100  @ double 256
?

If so, then why?

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3213: std::max(static_cast<size_t>(4),
typeAlignInBytes(Traits::IceType));
On 2015/09/18 13:59:34, stichnot wrote:
> First, I don't like magic constants like "4" sprinkled in the code.
> 
> Second, why make 4 the minimum alignment?  Can't an i16 constant pool be
aligned
> at 2-byte boundaries?

1) I avoid magic constants, too. However, this is way clearer (to me) than

static constexpr uint32_t MinimumWordAlign = 4;
std::max(MinimumWordAlign, typeAlignInBytes());

Named constants help a lot when the parameter is something "funny-looking" for
what you're trying to accomplish. (e.g. the far jump parameter on the X86
Assembler, which is a boolean -- it should have been an enum, at least.) In this
case, having a named constant just hides what the actual alignment is (plus, I
personally dislike the std::max(Minimum, Align(Type));

2) unaligned accesses are not guaranteed not to fault. As of right now, we
always (AFAIK) use a ldr (instead of the ldrb, ldrH variants), which would fault
if a magical alignment check bit is set (Murphy states that, if we allow
unaligned access, that bit will cause the program to fault.) Plus, because ARM,
everything is already so slow that I would like to prevent unaligned accesses.

[Jim Stichnoth, 2015-09-18 15:39:59]

lgtm

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3203: Str << "\t@" << Traits::TypeName << " " <<
Value << "\n";
On 2015/09/18 14:20:25, John wrote:
> On 2015/09/18 13:59:34, stichnot wrote:
> > Optional: It would be cool if, as an end-of-line comment, you could also
print
> > the non-hex FP representation of the constant.
> 
> I am already printing the actual FP value (e.g., 1.0). Do you mean outputting
> something like:
> 
> .quad 0x100  @ double 256
> ?
> 
> If so, then why?

Oh, whoops, sorry.
(sometimes I do prefer printf style format strings...)

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3213: std::max(static_cast<size_t>(4),
typeAlignInBytes(Traits::IceType));
On 2015/09/18 14:20:25, John wrote:
> On 2015/09/18 13:59:34, stichnot wrote:
> > First, I don't like magic constants like "4" sprinkled in the code.
> > 
> > Second, why make 4 the minimum alignment?  Can't an i16 constant pool be
> aligned
> > at 2-byte boundaries?
> 
> 1) I avoid magic constants, too. However, this is way clearer (to me) than
> 
> static constexpr uint32_t MinimumWordAlign = 4;
> std::max(MinimumWordAlign, typeAlignInBytes());
> 
> Named constants help a lot when the parameter is something "funny-looking" for
> what you're trying to accomplish. (e.g. the far jump parameter on the X86
> Assembler, which is a boolean -- it should have been an enum, at least.) In
this
> case, having a named constant just hides what the actual alignment is (plus, I
> personally dislike the std::max(Minimum, Align(Type));
> 
> 2) unaligned accesses are not guaranteed not to fault. As of right now, we
> always (AFAIK) use a ldr (instead of the ldrb, ldrH variants), which would
fault
> if a magical alignment check bit is set (Murphy states that, if we allow
> unaligned access, that bit will cause the program to fault.) Plus, because
ARM,
> everything is already so slow that I would like to prevent unaligned accesses.

I see, thanks.

[John, 2015-09-18 15:42:29]

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1347223003/diff/20001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:3203: Str << "\t@" << Traits::TypeName << " " <<
Value << "\n";
On 2015/09/18 15:39:59, stichnot wrote:
> On 2015/09/18 14:20:25, John wrote:
> > On 2015/09/18 13:59:34, stichnot wrote:
> > > Optional: It would be cool if, as an end-of-line comment, you could also
> print
> > > the non-hex FP representation of the constant.
> > 
> > I am already printing the actual FP value (e.g., 1.0). Do you mean
outputting
> > something like:
> > 
> > .quad 0x100  @ double 256
> > ?
> > 
> > If so, then why?
> 
> Oh, whoops, sorry.
> (sometimes I do prefer printf style format strings...)

I always prefer printf format string -- they are more readable. But this
implementation is more efficient (and not awfully unreadable -- just a tad bit
:) )

[John, 2015-09-18 15:43:07]

Committed patchset #4 (id:50001) manually as
a83bfde6f9b3fedd4503f55e0757674145f5b95a (presubmit successful).