Inline memove for small constant sizes and refactor memcpy and memset.

Inline memove for small constant sizes and refactor memcpy and memset.

The memory intrinsics are only optimized at -O1 and higher unless the
-fmem-intrin-opt flag is set to force to optimization to take place.

This change also introduces the xchg instruction for two register operands. This
is no longer used in the memory intrinsic lowering (or by anything else) but the
implementation is left for future use.

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

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

[ascull, 2015-08-11 21:45:29]

ascull@google.com changed reviewers:
+ jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[ascull, 2015-08-11 21:45:30]

None of spec2k uses memmove with a constant size so it isn't thoroughly tested.

I was unsure about the emitRex in the assembler. Have I used it correctly?

[Jim Stichnoth, 2015-08-12 13:41:47]

Be sure to get John's feedback on the rex prefix question.

Also, I think it's time to extend the mem_intrin cross tests to include most or
all of the interesting combinations implemented here, using xmacros+templates to
"auto-generate" the tests.

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

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3480: Type
TargetX86Base<Machine>::typeForSize(uint32_t Size, bool Overflow,
Is there anything machine-specific about this function?  If not, maybe move it
to IceTypes.cpp.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3489: LimitWidth ?
llvm::findLastSet(LimitWidth, llvm::ZB_Undefined) : 4;
Can you use llvm::array_lengthof(TypeForSize)-1 instead of 4?
(assuming that's correct and what's meant)

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3540: // the remainder will be at the end,
there will be reduces pressure on the
reduced (I think)

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3541: // memory unit as the access to the
same memory are far apart.
accesses ?

https://codereview.chromium.org/1278173009/diff/1/tests_lit/llvm2ice_tests/na...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://codereview.chromium.org/1278173009/diff/1/tests_lit/llvm2ice_tests/na...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:249: ; CHECK: mov WORD PTR
[{{.*}}+0xc],[[REG]]
Is this a CHECK instead of CHECK-NEXT because sandboxing?  If so, add a comment.

Also, to maximize the chances of the lowering sequence fitting into a bundle,
put a bogus call before the memmove call, so that the memmove lowering sequence
is forced to start at a new bundle boundary.  (But note that adding a call may
make register allocation harder/worse.)

It's unlikely that future changes separate from memmove lowering would increase
the code size, but if they accidentally do and that causes memmove spillover
into a new bundle, then I guess these tests would be a canary for that.

[John, 2015-08-12 17:44:58]

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86Base.h#n...
src/IceAssemblerX86Base.h:833: void xchg(Type Ty, typename Traits::GPRRegister
reg0,
Can you add tests for this new xchg instruction? They live in

unittest/AssemblerX8632/Locked.cpp

and

unittest/AssemblerX8664/Locked.cpp

to run the tests you need to

make CHECK_X86_ASM=1 check.

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl.h
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl...
src/IceAssemblerX86BaseImpl.h:3110: emitUint8(0x90 + reg1);
emitUint8(0x90 + gprEncoding(reg1));

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl...
src/IceAssemblerX86BaseImpl.h:3113: emitUint8(0x90 + reg0);
emitUint8(0x90 + gprEncoding(reg0));

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:159: Type typeForSize(uint32_t Size, bool
overflow = false,
Can this method be made static?

This comment tells me that you have 2 functions inside this one:

typeForSize(args, Overflow) {
  if Overflow:
    return largestTypeSmallerThanSize(args)
  else:
    return firstTypeThatFits(args)

The fact that, at every call site overflow is always a constant is another
pretty strong indication that you want two different functions.

I would also defined a constant named UnlimitedWidth (or NoWidthLimit) and use
it instead of = 0.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:161: /// Copy memory of given type from Src to
Dest using OffsetAmt on both.
the lower "prefix" threw me off here. I though there were a high level CopyMem
instruction. What do you thing about renaming this copyMemory?

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

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3484: IceType_f64, IceType_v16i8};
See if

Traits::Is64Bit ? IceType_i64 : IceType_f64

works for 64 bit data types. If it does, than please use that instead.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3511: _storep(Data, DestMem);
just curious: why store[pq]\? instead of mov?

you could also do something like:

if (isVectorType()) {
  _movp()
  _storep()
} else if (Ty == i64) {
  _movq()
  _storeq()
} else {
  _mov()
  _store()
}

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3545: uint32_t RemainingBytes = CountValue;
do you really need uint32_t's here? unless you really need the extra bit, please
use signed integers.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3560: Offset = CountValue -
typeWidthInBytes(LeftOverTy);
be careful with unaligned memory accesses. it might be better so simply copy
bytes?

[ascull, 2015-08-17 22:18:53]

I have added cross tests for different sized. It ugly and verbose so if you can
suggest a neater way to do it I would be happy.

The cross test showed I had implemented memmove incorrectly (I always copied
from highest to lowest address which meant I was copying in the wrong direction
sometimes *facepalm*).

I have added tests for the new xchg between registers instruction although I now
no longer use it. Should it stay around or should I remove it?

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86Base.h
File src/IceAssemblerX86Base.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86Base.h#n...
src/IceAssemblerX86Base.h:833: void xchg(Type Ty, typename Traits::GPRRegister
reg0,
On 2015/08/12 17:44:57, John wrote:
> Can you add tests for this new xchg instruction? They live in
> 
> unittest/AssemblerX8632/Locked.cpp
> 
> and
> 
> unittest/AssemblerX8664/Locked.cpp
> 
> to run the tests you need to
> 
> make CHECK_X86_ASM=1 check.

I have put some tests. I don't know how extensive you want them to be. I also
don't use this instruction any more so should it stay or be removed?

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl.h
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl...
src/IceAssemblerX86BaseImpl.h:3110: emitUint8(0x90 + reg1);
On 2015/08/12 17:44:57, John wrote:
> emitUint8(0x90 + gprEncoding(reg1));

Done.

https://codereview.chromium.org/1278173009/diff/1/src/IceAssemblerX86BaseImpl...
src/IceAssemblerX86BaseImpl.h:3113: emitUint8(0x90 + reg0);
On 2015/08/12 17:44:57, John wrote:
> emitUint8(0x90 + gprEncoding(reg0));

Done. Incl also only does `+ reg` is that a problem?

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:159: Type typeForSize(uint32_t Size, bool
overflow = false,
On 2015/08/12 17:44:57, John wrote:
> Can this method be made static?
> 
> This comment tells me that you have 2 functions inside this one:
> 
> typeForSize(args, Overflow) {
>   if Overflow:
>     return largestTypeSmallerThanSize(args)
>   else:
>     return firstTypeThatFits(args)
> 
> The fact that, at every call site overflow is always a constant is another
> pretty strong indication that you want two different functions.
> 
> I would also defined a constant named UnlimitedWidth (or NoWidthLimit) and use
> it instead of = 0.

Done.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:161: /// Copy memory of given type from Src to
Dest using OffsetAmt on both.
On 2015/08/12 17:44:57, John wrote:
> the lower "prefix" threw me off here. I though there were a high level CopyMem
> instruction. What do you thing about renaming this copyMemory?

Done.

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

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3480: Type
TargetX86Base<Machine>::typeForSize(uint32_t Size, bool Overflow,
On 2015/08/12 13:41:47, stichnot wrote:
> Is there anything machine-specific about this function?  If not, maybe move it
> to IceTypes.cpp.

Possibly, as we discussed.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3484: IceType_f64, IceType_v16i8};
On 2015/08/12 17:44:57, John wrote:
> See if
> 
> Traits::Is64Bit ? IceType_i64 : IceType_f64
> 
> works for 64 bit data types. If it does, than please use that instead.

That does work. And it sorts on of the TODOs, thanks.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3489: LimitWidth ?
llvm::findLastSet(LimitWidth, llvm::ZB_Undefined) : 4;
On 2015/08/12 13:41:47, stichnot wrote:
> Can you use llvm::array_lengthof(TypeForSize)-1 instead of 4?
> (assuming that's correct and what's meant)

This is the function I tried and failed to find, Thanks!
Done.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3511: _storep(Data, DestMem);
On 2015/08/12 17:44:57, John wrote:
> just curious: why store[pq]\? instead of mov?
> 
> you could also do something like:
> 
> if (isVectorType()) {
>   _movp()
>   _storep()
> } else if (Ty == i64) {
>   _movq()
>   _storeq()
> } else {
>   _mov()
>   _store()
> }

_store seems to be needed for writing to memory. I guess it's just to make it
clearer to read instead of trying to decipher the mov.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3540: // the remainder will be at the end,
there will be reduces pressure on the
On 2015/08/12 13:41:47, stichnot wrote:
> reduced (I think)

Done.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3541: // memory unit as the access to the
same memory are far apart.
On 2015/08/12 13:41:47, stichnot wrote:
> accesses ?

Done.

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3545: uint32_t RemainingBytes = CountValue;
On 2015/08/12 17:44:57, John wrote:
> do you really need uint32_t's here? unless you really need the extra bit,
please
> use signed integers.

The CountValue comes from a size_t so is unsigned. I've changes Offset to int_32
to match getConstantInt32. What do you prefer signed?

https://codereview.chromium.org/1278173009/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3560: Offset = CountValue -
typeWidthInBytes(LeftOverTy);
On 2015/08/12 17:44:57, John wrote:
> be careful with unaligned memory accesses. it might be better so simply copy
> bytes?

This is the way gcc and clang do it. Since the alignment will not be known there
will be a large overhead in checking that. Doing it with bytes seems like it
would take longer, especially if there are 15 bytes! We also don't know if the
previous copies were aligned (hence the use of movups).

https://codereview.chromium.org/1278173009/diff/1/tests_lit/llvm2ice_tests/na...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://codereview.chromium.org/1278173009/diff/1/tests_lit/llvm2ice_tests/na...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:249: ; CHECK: mov WORD PTR
[{{.*}}+0xc],[[REG]]
On 2015/08/12 13:41:47, stichnot wrote:
> Is this a CHECK instead of CHECK-NEXT because sandboxing?  If so, add a
comment.
> 
> Also, to maximize the chances of the lowering sequence fitting into a bundle,
> put a bogus call before the memmove call, so that the memmove lowering
sequence
> is forced to start at a new bundle boundary.  (But note that adding a call may
> make register allocation harder/worse.)
> 
> It's unlikely that future changes separate from memmove lowering would
increase
> the code size, but if they accidentally do and that causes memmove spillover
> into a new bundle, then I guess these tests would be a canary for that.

With the new (and correct) implementation on memmove this test is not longer of
interest so has been removed. I'll keep this trick in mind for use in future
though.

[ascull, 2015-08-18 16:53:58]

X-macros to simplify the cross tests and flags to control memory intrinsic
optimizations.

[Jim Stichnoth, 2015-08-18 17:11:28]

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
Cool.

Maybe add "check-unit" to the echo string, so that "make check" output is more
understandable?

https://codereview.chromium.org/1278173009/diff/40001/crosstest/crosstest.cfg
File crosstest/crosstest.cfg (right):

https://codereview.chromium.org/1278173009/diff/40001/crosstest/crosstest.cfg...
crosstest/crosstest.cfg:8: flags: --sz-args='-fmem-intrin-opt'
Can you drop the single quotes?  The fact that this actually works suggests that
the shell is getting involved in command execution; it would surely break if the
quote characters weren't stripped by the shell; and quotes shouldn't be needed
even with the shell.

https://codereview.chromium.org/1278173009/diff/40001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/1278173009/diff/40001/pydir/crosstest.py#newc...
pydir/crosstest.py:96: argparser.add_argument('--sz-args', required=False,
default='',
Can you name this '--sz' to match szbuild.py?

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3528: if ((Ctx->getFlags().getOptLevel() >=
Opt_1 ||
Refactor this new condition into a function in an anonymous namespace.

https://codereview.chromium.org/1278173009/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://codereview.chromium.org/1278173009/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:7: ; RUN:   --target x8632 -i %s
--args -Om1 --fmem-intrin-opt -sandbox \
I think you should have one more RUN configuration that tests Om1 without
-fmem-intrin-opt.  The CHECK lines would look pretty much like the ARM checks.

[jvoung (off chromium), 2015-08-18 17:18:23]

https://codereview.chromium.org/1278173009/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/20001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
Thanks =)

Would it make sense to put this by the "ifdef CHECK_X86_ASM" block instead, so
that at least other unittests could run w/ or without DEBUG?

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

https://codereview.chromium.org/1278173009/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:156: // Load from memory for a given type.
Did you intend to doxygenize this (and typedStore) too or leave as "//" ?

https://codereview.chromium.org/1278173009/diff/40001/crosstest/mem_intrin.def
File crosstest/mem_intrin.def (right):

https://codereview.chromium.org/1278173009/diff/40001/crosstest/mem_intrin.de...
crosstest/mem_intrin.def:1: #define VALUES \
better =)

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3599: // so make surenot to clobber the
memory. This also means overlapping moves
"surenot" -> "sure not"

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3709: // instructions so are limits to 32-bit
stores.
"so are limited" ?

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8632...
File unittest/AssemblerX8632/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8632...
unittest/AssemblerX8632/Locked.cpp:135: #undef TestImplAddrReg
undef TestImplRegReg

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
File unittest/AssemblerX8664/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:119: TestImplSize(Reg0, Reg1, 32);           
                                  \
Does size 64 work?

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:122: TestImpl(r1, r8);
Maybe throw in eax/rax too, if that has a special encoding.

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:132: #undef TestImplAddrReg
undef TestImplRegReg

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

https://codereview.chromium.org/1278173009/diff/20001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/20001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
On 2015/08/18 17:18:22, jvoung wrote:
> Thanks =)
> 
> Would it make sense to put this by the "ifdef CHECK_X86_ASM" block instead, so
> that at least other unittests could run w/ or without DEBUG?

Done.

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

https://codereview.chromium.org/1278173009/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:156: // Load from memory for a given type.
On 2015/08/18 17:18:22, jvoung wrote:
> Did you intend to doxygenize this (and typedStore) too or leave as "//" ?

Done.

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
On 2015/08/18 17:11:28, stichnot wrote:
> Cool.
> 
> Maybe add "check-unit" to the echo string, so that "make check" output is more
> understandable?

Done.

https://codereview.chromium.org/1278173009/diff/40001/crosstest/crosstest.cfg
File crosstest/crosstest.cfg (right):

https://codereview.chromium.org/1278173009/diff/40001/crosstest/crosstest.cfg...
crosstest/crosstest.cfg:8: flags: --sz-args='-fmem-intrin-opt'
On 2015/08/18 17:11:28, stichnot wrote:
> Can you drop the single quotes?  The fact that this actually works suggests
that
> the shell is getting involved in command execution; it would surely break if
the
> quote characters weren't stripped by the shell; and quotes shouldn't be needed
> even with the shell.

Done.

https://codereview.chromium.org/1278173009/diff/40001/crosstest/mem_intrin.def
File crosstest/mem_intrin.def (right):

https://codereview.chromium.org/1278173009/diff/40001/crosstest/mem_intrin.de...
crosstest/mem_intrin.def:1: #define VALUES \
On 2015/08/18 17:18:22, jvoung wrote:
> better =)

Credit goes to Jim.

https://codereview.chromium.org/1278173009/diff/40001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/1278173009/diff/40001/pydir/crosstest.py#newc...
pydir/crosstest.py:96: argparser.add_argument('--sz-args', required=False,
default='',
On 2015/08/18 17:11:28, stichnot wrote:
> Can you name this '--sz' to match szbuild.py?

Done.

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3528: if ((Ctx->getFlags().getOptLevel() >=
Opt_1 ||
On 2015/08/18 17:11:28, stichnot wrote:
> Refactor this new condition into a function in an anonymous namespace.

I put this is TargetLowering as it needs access to the flags through Ctx and it
is general across targets.

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3599: // so make surenot to clobber the
memory. This also means overlapping moves
On 2015/08/18 17:18:22, jvoung wrote:
> "surenot" -> "sure not"

Done.

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3709: // instructions so are limits to 32-bit
stores.
On 2015/08/18 17:18:22, jvoung wrote:
> "so are limited" ?

Done.

https://codereview.chromium.org/1278173009/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll (right):

https://codereview.chromium.org/1278173009/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-mem-intrinsics.ll:7: ; RUN:   --target x8632 -i %s
--args -Om1 --fmem-intrin-opt -sandbox \
On 2015/08/18 17:11:28, stichnot wrote:
> I think you should have one more RUN configuration that tests Om1 without
> -fmem-intrin-opt.  The CHECK lines would look pretty much like the ARM checks.

Done.

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8632...
File unittest/AssemblerX8632/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8632...
unittest/AssemblerX8632/Locked.cpp:135: #undef TestImplAddrReg
On 2015/08/18 17:18:22, jvoung wrote:
> undef TestImplRegReg

Done.

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
File unittest/AssemblerX8664/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:119: TestImplSize(Reg0, Reg1, 32);           
                                  \
On 2015/08/18 17:18:22, jvoung wrote:
> Does size 64 work?

No. I tried it and there were a lot of build errors I didn't understand.

JPP: do you know anything about this?

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:122: TestImpl(r1, r8);
On 2015/08/18 17:18:22, jvoung wrote:
> Maybe throw in eax/rax too, if that has a special encoding.

Done.

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:132: #undef TestImplAddrReg
On 2015/08/18 17:18:22, jvoung wrote:
> undef TestImplRegReg

Done.

[John, 2015-08-18 18:51:39]

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
On 2015/08/18 17:11:28, stichnot wrote:
> Cool.
> 
> Maybe add "check-unit" to the echo string, so that "make check" output is more
> understandable?

I have something similar in a patch -- which I reproduce here:
UNITTEST_SRCS = \
        BitcodeMunge.cpp \
        IceELFSectionTest.cpp \
        IceParseInstsTest.cpp

# PATCH-BEGIN
# The X86 assembler tests take too long to compile. Given how infrequently the
# assembler will change, we disable them. Furthermore, they are completely
# disabled in non-DEBUG builds. The compiler goes bananas with so many
# optimization oportunities, and eventually freezes the entire machine.
ifdef CHECK_X86_ASM
ifndef DEBUG
$(info X86 Assembler tests not available in non-debug build.)
else
UNITTEST_SRCS += AssemblerX8632/LowLevel.cpp \
        AssemblerX8632/DataMov.cpp \
        AssemblerX8632/Locked.cpp \
        AssemblerX8632/GPRArith.cpp \
        AssemblerX8632/XmmArith.cpp \
        AssemblerX8632/ControlFlow.cpp \
        AssemblerX8632/Other.cpp \
        AssemblerX8632/X87.cpp \
        AssemblerX8664/LowLevel.cpp \
        AssemblerX8664/DataMov.cpp \
        AssemblerX8664/Locked.cpp \
        AssemblerX8664/GPRArith.cpp \
        AssemblerX8664/XmmArith.cpp \
        AssemblerX8664/ControlFlow.cpp \
        AssemblerX8664/Other.cpp
endif
endif
# PATCH-END

UNITTEST_OBJS = $(patsubst %.cpp, $(OBJDIR)/unittest/%.o, $(UNITTEST_SRCS))

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3528: if ((Ctx->getFlags().getOptLevel() >=
Opt_1 ||
On 2015/08/18 17:11:28, stichnot wrote:
> Refactor this new condition into a function in an anonymous namespace.

I second the motion, but don't create an anonymous namespace. This file is a
header, and anonymous namespaces in header files will cause problems. :)

Just declare a global function (or a static member function):

bool shouldOptimizeMemIntrin(Flags) {
}

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
File unittest/AssemblerX8664/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:122: TestImpl(r1, r8);
On 2015/08/18 17:18:22, jvoung wrote:
> Maybe throw in eax/rax too, if that has a special encoding.

r1 == rax

Sorry for making this so confusing.

[ascull, 2015-08-18 19:03:10]

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1278173009/diff/40001/Makefile.standalone#new...
Makefile.standalone:369: @echo "Run with DEBUG=1 lest your machine perish."
On 2015/08/18 18:51:39, John wrote:
> On 2015/08/18 17:11:28, stichnot wrote:
> > Cool.
> > 
> > Maybe add "check-unit" to the echo string, so that "make check" output is
more
> > understandable?
> 
> I have something similar in a patch -- which I reproduce here:
> UNITTEST_SRCS = \
>         BitcodeMunge.cpp \
>         IceELFSectionTest.cpp \
>         IceParseInstsTest.cpp
> 
> # PATCH-BEGIN
> # The X86 assembler tests take too long to compile. Given how infrequently the
> # assembler will change, we disable them. Furthermore, they are completely
> # disabled in non-DEBUG builds. The compiler goes bananas with so many
> # optimization oportunities, and eventually freezes the entire machine.
> ifdef CHECK_X86_ASM
> ifndef DEBUG
> $(info X86 Assembler tests not available in non-debug build.)
> else
> UNITTEST_SRCS += AssemblerX8632/LowLevel.cpp \
>         AssemblerX8632/DataMov.cpp \
>         AssemblerX8632/Locked.cpp \
>         AssemblerX8632/GPRArith.cpp \
>         AssemblerX8632/XmmArith.cpp \
>         AssemblerX8632/ControlFlow.cpp \
>         AssemblerX8632/Other.cpp \
>         AssemblerX8632/X87.cpp \
>         AssemblerX8664/LowLevel.cpp \
>         AssemblerX8664/DataMov.cpp \
>         AssemblerX8664/Locked.cpp \
>         AssemblerX8664/GPRArith.cpp \
>         AssemblerX8664/XmmArith.cpp \
>         AssemblerX8664/ControlFlow.cpp \
>         AssemblerX8664/Other.cpp
> endif
> endif
> # PATCH-END
> 
> UNITTEST_OBJS = $(patsubst %.cpp, $(OBJDIR)/unittest/%.o, $(UNITTEST_SRCS))
> 

We can merge to yours when the time comes. Mine is a bit more aggressive.

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3528: if ((Ctx->getFlags().getOptLevel() >=
Opt_1 ||
On 2015/08/18 18:51:39, John wrote:
> On 2015/08/18 17:11:28, stichnot wrote:
> > Refactor this new condition into a function in an anonymous namespace.
> 
> I second the motion, but don't create an anonymous namespace. This file is a
> header, and anonymous namespaces in header files will cause problems. :)
> 
> Just declare a global function (or a static member function):
> 
> bool shouldOptimizeMemIntrin(Flags) {
> }

I don't think static works either as it needs access to the flags.

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
File unittest/AssemblerX8664/Locked.cpp (right):

https://codereview.chromium.org/1278173009/diff/40001/unittest/AssemblerX8664...
unittest/AssemblerX8664/Locked.cpp:122: TestImpl(r1, r8);
On 2015/08/18 18:51:39, John wrote:
> On 2015/08/18 17:18:22, jvoung wrote:
> > Maybe throw in eax/rax too, if that has a special encoding.
> 
> r1 == rax
> 
> Sorry for making this so confusing.

Ok, I've fixed the fix ;)

[John, 2015-08-18 19:34:07]

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

https://codereview.chromium.org/1278173009/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3528: if ((Ctx->getFlags().getOptLevel() >=
Opt_1 ||
On 2015/08/18 19:03:10, ascull wrote:
> On 2015/08/18 18:51:39, John wrote:
> > On 2015/08/18 17:11:28, stichnot wrote:
> > > Refactor this new condition into a function in an anonymous namespace.
> > 
> > I second the motion, but don't create an anonymous namespace. This file is a
> > header, and anonymous namespaces in header files will cause problems. :)
> > 
> > Just declare a global function (or a static member function):
> > 
> > bool shouldOptimizeMemIntrin(Flags) {
> > }
> 
> I don't think static works either as it needs access to the flags.

that's why you'd pass it as a parameter. :)

[jvoung (off chromium), 2015-08-20 15:42:53]

LGTM -- but check with John and Jim if they have other comments

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

https://codereview.chromium.org/1278173009/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3575: constexpr uint32_t UNROLL_LIMIT = 8; //
32-bit has 8 xmm registers
I'm not sure the exact side effects on register allocation, if we end up using
all the 32-bit registers, but I suppose a function call would have clobbered all
8 too. I.e., wondering if for 32-bit targets keep it lower like 6 or 7, and for
64-bit targets let it be 8.

[Jim Stichnoth, 2015-08-20 16:57:05]

otherwise lgtm

https://codereview.chromium.org/1278173009/diff/80001/crosstest/mem_intrin.def
File crosstest/mem_intrin.def (right):

https://codereview.chromium.org/1278173009/diff/80001/crosstest/mem_intrin.de...
crosstest/mem_intrin.def:1: #define VALUES \
I should have mentioned this before, but it would probably be better to have a
more descriptive name than VALUES.  In the rest of the code base, most of the
macro names end with _TABLE, so maybe something like MEMINTRIN_SIZE_TABLE?

Also, how about adding X(0)?

https://codereview.chromium.org/1278173009/diff/80001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/1278173009/diff/80001/pydir/crosstest.py#newc...
pydir/crosstest.py:96: argparser.add_argument('--sz', required=False,
default='',
This should be made to allow multiple --sz arguments, like in szbuild.py:

  add_argument('--sz', dest='sz_args', action='append', default=[], help='...')

then add the arg array appropriately in the shellcmd.

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3146: const auto VarReg1 =
llvm::cast<Variable>(this->getSrc(1));
const auto *

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3152: if (const auto VarReg0 =
llvm::dyn_cast<Variable>(this->getSrc(0))) {
const auto *

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3161: const auto Mem =
const auto *

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

https://codereview.chromium.org/1278173009/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3575: constexpr uint32_t UNROLL_LIMIT = 8; //
32-bit has 8 xmm registers
On 2015/08/20 15:42:53, jvoung wrote:
> I'm not sure the exact side effects on register allocation, if we end up using
> all the 32-bit registers, but I suppose a function call would have clobbered
all
> 8 too. I.e., wondering if for 32-bit targets keep it lower like 6 or 7, and
for
> 64-bit targets let it be 8.

Could also get the limit from the Machine traits or whatever, instead of locking
it down here.

https://codereview.chromium.org/1278173009/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4959: uint32_t MaxIndex = MaxSize !=
NoSizeLimit
I think it's generally easier to read if you minimize the double negatives.

  MaxIndex = MaxSize == NoSizeLimit ? array_lengthof() : findLastSet();

(here and below)

[ascull, 2015-08-20 18:41:56]

https://codereview.chromium.org/1278173009/diff/80001/crosstest/mem_intrin.def
File crosstest/mem_intrin.def (right):

https://codereview.chromium.org/1278173009/diff/80001/crosstest/mem_intrin.de...
crosstest/mem_intrin.def:1: #define VALUES \
On 2015/08/20 16:57:04, stichnot wrote:
> I should have mentioned this before, but it would probably be better to have a
> more descriptive name than VALUES.  In the rest of the code base, most of the
> macro names end with _TABLE, so maybe something like MEMINTRIN_SIZE_TABLE?
> 
> Also, how about adding X(0)?

Done.

https://codereview.chromium.org/1278173009/diff/80001/pydir/crosstest.py
File pydir/crosstest.py (right):

https://codereview.chromium.org/1278173009/diff/80001/pydir/crosstest.py#newc...
pydir/crosstest.py:96: argparser.add_argument('--sz', required=False,
default='',
On 2015/08/20 16:57:04, stichnot wrote:
> This should be made to allow multiple --sz arguments, like in szbuild.py:
> 
>   add_argument('--sz', dest='sz_args', action='append', default=[],
help='...')
> 
> then add the arg array appropriately in the shellcmd.

Done.

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3146: const auto VarReg1 =
llvm::cast<Variable>(this->getSrc(1));
On 2015/08/20 16:57:04, stichnot wrote:
> const auto *

Done.

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3152: if (const auto VarReg0 =
llvm::dyn_cast<Variable>(this->getSrc(0))) {
On 2015/08/20 16:57:04, stichnot wrote:
> const auto *

Done.

https://codereview.chromium.org/1278173009/diff/80001/src/IceInstX86BaseImpl....
src/IceInstX86BaseImpl.h:3161: const auto Mem =
On 2015/08/20 16:57:04, stichnot wrote:
> const auto *

Done.

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

https://codereview.chromium.org/1278173009/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:3575: constexpr uint32_t UNROLL_LIMIT = 8; //
32-bit has 8 xmm registers
On 2015/08/20 16:57:04, stichnot wrote:
> On 2015/08/20 15:42:53, jvoung wrote:
> > I'm not sure the exact side effects on register allocation, if we end up
using
> > all the 32-bit registers, but I suppose a function call would have clobbered
> all
> > 8 too. I.e., wondering if for 32-bit targets keep it lower like 6 or 7, and
> for
> > 64-bit targets let it be 8.
> 
> Could also get the limit from the Machine traits or whatever, instead of
locking
> it down here.

I put it in traits and left the value as is for now. Memmove is uncommon and the
use of xmm registers is also less common so it shouldn't cause too much of an
issue. It's easy to change if it does though.

https://codereview.chromium.org/1278173009/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4959: uint32_t MaxIndex = MaxSize !=
NoSizeLimit
On 2015/08/20 16:57:04, stichnot wrote:
> I think it's generally easier to read if you minimize the double negatives.
> 
>   MaxIndex = MaxSize == NoSizeLimit ? array_lengthof() : findLastSet();
> 
> (here and below)

Done.

[ascull, 2015-08-20 21:23:13]

Committed patchset #9 (id:160001) manually as
cfa628b5f7612d202de234980b21bd6d59a11998 (presubmit successful).