Inline memset when there is a constant value and count.

Inline memset when there is a constant value and count.

BUG=
R=jvoung@chromium.org, jvoung, stichnot

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

[ascull, 2015-08-04 00:33:08]

Eon has shown promise of ~5% improvement. Others don't seem to have
changed much (eon also has a lot of memcpy which I will be targeting
next).

There are a ton of little cases to handle and I tried my best to
simplify the code.

[jvoung (off chromium), 2015-08-04 16:50:16]

Thanks!

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3981: // TODO(ascull): if const count but reg
val could inline too with subregs
Could you expand on this comment more, in case it's forgotten what the problem
with subregs is?

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3987: // 3 is the awkward size as it is too
small for the vector or 32-bit
lowerleftOvers -> lowerLeftOvers

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3996: Offset =
Ctx->getConstantInt(Base->getType(), 2);
Hmm, getConstantInt() vs getConstantInt32 is to make this pointer-size agnostic?
I think that's good to consider =)

... but perhaps we don't need it to be so general.

Looking into this more, on x86-64 with 64-bit pointers, while displacement can
be variable sized from 1 byte, 2,  4 bytes, or 8 bytes, there is a restriction
on 8 bytes. For 8-byte displacement, looks like this is the "Direct
Memory-Offset MOVs" which have a special form, and pretty much only allow eax?

cat test.s
movabsl 0x123456789a, %eax
movabsq 0x123456789a, %rax
movabsq 0x123456789a, %rdx

clang -c test.s -o test.o && otool -t test.o -V -j
test.s:3:23: error: invalid operand for instruction
movabsq 0x123456789a, %rdx
                      ^~~~

Also it seems unlikely for us to want to inline/unroll so many instructions that
the displacement grows up to 64-bit =)

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4007: auto lowerStoreZero = [this, Base,
CountValue, Value](Type Ty) {
Is this more general than lowerStoreZero now? lowerLeftOvers is given a
"uint32_t Value" which doesn't seem restricted to being 0.

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4045: auto *Mem =
Traits::X86OperandMem::create(Func, Zero->getType(), Base,
Hmm, maybe use IceType_i64 instead of Zero->getType() since we are storing to a
"qword ptr" and not an "xmmword ptr". with storeq.

[ascull, 2015-08-04 18:01:01]

There's also a rebase mixed in there, sorry!

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3981: // TODO(ascull): if const count but reg
val could inline too with subregs
On 2015/08/04 16:50:16, jvoung wrote:
> Could you expand on this comment more, in case it's forgotten what the problem
> with subregs is?

Done.

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3987: // 3 is the awkward size as it is too
small for the vector or 32-bit
On 2015/08/04 16:50:16, jvoung wrote:
> lowerleftOvers -> lowerLeftOvers

Done.

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3996: Offset =
Ctx->getConstantInt(Base->getType(), 2);
On 2015/08/04 16:50:16, jvoung wrote:
> Hmm, getConstantInt() vs getConstantInt32 is to make this pointer-size
agnostic?
> I think that's good to consider =)
> 
> ... but perhaps we don't need it to be so general.
> 
> Looking into this more, on x86-64 with 64-bit pointers, while displacement can
> be variable sized from 1 byte, 2,  4 bytes, or 8 bytes, there is a restriction
> on 8 bytes. For 8-byte displacement, looks like this is the "Direct
> Memory-Offset MOVs" which have a special form, and pretty much only allow eax?
> 
> cat test.s
> movabsl 0x123456789a, %eax
> movabsq 0x123456789a, %rax
> movabsq 0x123456789a, %rdx
> 
> clang -c test.s -o test.o && otool -t test.o -V -j
> test.s:3:23: error: invalid operand for instruction
> movabsq 0x123456789a, %rdx
>                       ^~~~
> 
> Also it seems unlikely for us to want to inline/unroll so many instructions
that
> the displacement grows up to 64-bit =)

I thought I remembered that the size of Base and Offset had to be equal
but that doesn't seem to be that case. I've changed these using the
knowledge that Count will be 32-bit at the largest.

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4007: auto lowerStoreZero = [this, Base,
CountValue, Value](Type Ty) {
On 2015/08/04 16:50:16, jvoung wrote:
> Is this more general than lowerStoreZero now? lowerLeftOvers is given a
> "uint32_t Value" which doesn't seem restricted to being 0.

It is more general but I forgot to update the name. Done.

https://codereview.chromium.org/1255053008/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4045: auto *Mem =
Traits::X86OperandMem::create(Func, Zero->getType(), Base,
On 2015/08/04 16:50:16, jvoung wrote:
> Hmm, maybe use IceType_i64 instead of Zero->getType() since we are storing to
a
> "qword ptr" and not an "xmmword ptr". with storeq.

Done.

[jvoung (off chromium), 2015-08-04 21:17:36]

LGTM thanks

https://codereview.chromium.org/1255053008/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1255053008/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4061: if (ValValue == 0 && CountValue >= 8 &&
CountValue <= 16 * UNROLL_LIMIT) {
Would it make sense/look good to "document" the "16" here and the "4" below
which are part of N * UNROLL_LIMIT as "constexpr uint32_t BytesPerStore = N" or
something like that?

[ascull, 2015-08-04 21:25:30]

Committed patchset #3 (id:40001) manually as
713dbdecbdd74f77cbb1cae43b19bdd6f6b3144f (presubmit successful).