Add option to force filetype=asm for testing

Add option to force filetype=asm for testing

Also turn it on for presubmit testing.

BUG=
R=stichnot@chromium.org

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

[sehr, 2015-12-18 01:24:56]

Description was changed from

==========
Add option to force filetype=asm for testing

Also turn it on for presubmit testing.

BUG=
==========

to

==========
Add option to force filetype=asm for testing

Also turn it on for presubmit testing.

BUG=
==========

[sehr, 2015-12-18 01:24:56]

sehr@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[sehr, 2015-12-18 01:25:05]

PTAL.

[Jim Stichnoth, 2015-12-20 18:42:23]

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

https://codereview.chromium.org/1531623007/diff/40001/Makefile.standalone#new...
Makefile.standalone:143: FORCEASM_XTEST_EXCLUDES = -e
x8632,sandbox,test_sync_atomic
Explain this, e.g.:

# With --filetype=asm and --sandbox, the llvm-mc assembler emits the lock and
16-bit prefixes in the "wrong" order, causing the validator to reject the
resulting nexe.  So we just disable those tests for now.

https://codereview.chromium.org/1531623007/diff/40001/Makefile.standalone#new...
Makefile.standalone:479: # Run cross tests to check for regressions in
filetype=asm
How about more positive phrasing:
  Run cross tests to validate filetype=asm output

:)

https://codereview.chromium.org/1531623007/diff/40001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:82: help='Force --filetype=asm.')
OCD: Remove the '.' from the help string for consistency.

And in the same vein:
  s/.  Default %(default)s.//
because formatter_class=argparse.ArgumentDefaultsHelpFormatter) makes that
unnecessary.

https://codereview.chromium.org/1531623007/diff/40001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:817: ArithmeticTypeOverride = ArithmeticType;
Can this assignment be moved into the ctor member initializer list?

If so, I would rename the parameter to ArithmeticTypeOverride, and declare the
field as const.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:214: ReturnReg = makeReg(Dest->getType(),
Traits::RegisterSet::Reg_eax);
Instead of the above, I think you could do something like:
  ReturnReg = makeReg(Dest->getType(), Traits::getGprForType(Dest->getType(),
Traits::RegisterSet::Reg_eax));

Also, it would probably be good to change all the Dest->getType() calls in this
method into DestTy=Dest->getType() .  And maybe do the same in the x8664 file
for symmetry.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:349: } else if (Src0->getType() == IceType_i8) {
Maybe check against i1 as well, for completeness

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:350: // TODO(sehr,stichnot): I think these need
to sign/zero extend.
I don't think the x86-32 processor ABI has anything to say on this matter.

So it comes down to consistency between the caller and callee.  Subzero should
be self-consistent in this regard.

But if Subzero output is linked against another translator's output, such as for
bisection debugging, this would be an issue.

Given that returning i1/i8/i16 isn't even valid in a pexe, I think it would be
fine to explicitly zero-extend.  In fact, that could potentially save debugging
time for some future non-PNaCl-based integration when we have long forgotten
this issue.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:370: if (Src0->getType() == IceType_i8) {
Same comment as in x86-32 - handle IceType_i1 as well

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:376: _mov(Reg, Src0,
Traits::RegisterSet::Reg_eax);
You probably need to deal with i32/eax and i64/rax as well, otherwise it will
likely break the x86-64 resurrection CL.

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

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:551: Type ArithmeticType = IceType_void) {
rename parameter to ArithmeticTypeOverride ?

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

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5752: // pcmpeqq requires SSE 4.1, while
pcmpeqd does the same job and only
Could you also note that f64 is (currently) used only for fabs intrinsic
lowering?  Otherwise I was having a hard time figuring out how this code path
was taken.

[sehr, 2016-01-07 18:53:12]

OK, it now works for all lit and xtest tests, and I've attempted to address
Jim's comments.

PTAL.

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

https://codereview.chromium.org/1531623007/diff/40001/Makefile.standalone#new...
Makefile.standalone:143: FORCEASM_XTEST_EXCLUDES = -e
x8632,sandbox,test_sync_atomic
On 2015/12/20 18:42:23, stichnot wrote:
> Explain this, e.g.:
> 
> # With --filetype=asm and --sandbox, the llvm-mc assembler emits the lock and
> 16-bit prefixes in the "wrong" order, causing the validator to reject the
> resulting nexe.  So we just disable those tests for now.

Done.

https://codereview.chromium.org/1531623007/diff/40001/Makefile.standalone#new...
Makefile.standalone:479: # Run cross tests to check for regressions in
filetype=asm
On 2015/12/20 18:42:23, stichnot wrote:
> How about more positive phrasing:
>   Run cross tests to validate filetype=asm output
> 
> :)

Done.

https://codereview.chromium.org/1531623007/diff/40001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:82: help='Force --filetype=asm.')
On 2015/12/20 18:42:23, stichnot wrote:
> OCD: Remove the '.' from the help string for consistency.
> 
> And in the same vein:
>   s/.  Default %(default)s.//
> because formatter_class=argparse.ArgumentDefaultsHelpFormatter) makes that
> unnecessary.

Done.

https://codereview.chromium.org/1531623007/diff/40001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:817: ArithmeticTypeOverride = ArithmeticType;
On 2015/12/20 18:42:23, stichnot wrote:
> Can this assignment be moved into the ctor member initializer list?
> 
> If so, I would rename the parameter to ArithmeticTypeOverride, and declare the
> field as const.

Done.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:214: ReturnReg = makeReg(Dest->getType(),
Traits::RegisterSet::Reg_eax);
On 2015/12/20 18:42:23, stichnot wrote:
> Instead of the above, I think you could do something like:
>   ReturnReg = makeReg(Dest->getType(), Traits::getGprForType(Dest->getType(),
> Traits::RegisterSet::Reg_eax));
> 
> Also, it would probably be good to change all the Dest->getType() calls in
this
> method into DestTy=Dest->getType() .  And maybe do the same in the x8664 file
> for symmetry.

Done.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:349: } else if (Src0->getType() == IceType_i8) {
On 2015/12/20 18:42:23, stichnot wrote:
> Maybe check against i1 as well, for completeness

I converted these all to use the getGprForType scheme.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:350: // TODO(sehr,stichnot): I think these need
to sign/zero extend.
On 2015/12/20 18:42:23, stichnot wrote:
> I don't think the x86-32 processor ABI has anything to say on this matter.
> 
> So it comes down to consistency between the caller and callee.  Subzero should
> be self-consistent in this regard.
> 
> But if Subzero output is linked against another translator's output, such as
for
> bisection debugging, this would be an issue.
> 
> Given that returning i1/i8/i16 isn't even valid in a pexe, I think it would be
> fine to explicitly zero-extend.  In fact, that could potentially save
debugging
> time for some future non-PNaCl-based integration when we have long forgotten
> this issue.

I'm going with the idea that we're doing this for pexes and it's not valid, so I
remove the comment.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:370: if (Src0->getType() == IceType_i8) {
On 2015/12/20 18:42:23, stichnot wrote:
> Same comment as in x86-32 - handle IceType_i1 as well

Done.

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8664.cpp:376: _mov(Reg, Src0,
Traits::RegisterSet::Reg_eax);
On 2015/12/20 18:42:23, stichnot wrote:
> You probably need to deal with i32/eax and i64/rax as well, otherwise it will
> likely break the x86-64 resurrection CL.

John beat me to the punch.  It should work there also.

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

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:551: Type ArithmeticType = IceType_void) {
On 2015/12/20 18:42:23, stichnot wrote:
> rename parameter to ArithmeticTypeOverride ?

Done.

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

https://codereview.chromium.org/1531623007/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5752: // pcmpeqq requires SSE 4.1, while
pcmpeqd does the same job and only
On 2015/12/20 18:42:23, stichnot wrote:
> Could you also note that f64 is (currently) used only for fabs intrinsic
> lowering?  Otherwise I was having a hard time figuring out how this code path
> was taken.

Done.

[Jim Stichnoth, 2016-01-10 03:08:56]

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone#ne...
Makefile.standalone:485: # Run spec2k for x86-32.
Can you fix this comment?  It's actually running lit tests, cross tests, unit
tests, and spec2k/x86-32.

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone#ne...
Makefile.standalone:496: check-xtest FORCEASM=1
Can you do this with a single sub-command?

+make -f Makefile.standalone FORCEASM=1 check-lit check-xtest

https://codereview.chromium.org/1531623007/diff/160001/pydir/crosstest_genera...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1531623007/diff/160001/pydir/crosstest_genera...
pydir/crosstest_generator.py:111: help='File type for compilation {asm, iasm,
obj}.')
I suggest documenting this something like "...file type override..."

https://codereview.chromium.org/1531623007/diff/160001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/160001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:44: help='output file to write')
capitalize Output

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:826: Type DestTy = this->getDest()->getType();
How about this:

  const Type DestTy = ArithmeticTypeOverride == IceType_void ?
this->getDest()->getType() : ArithmeticTypeOverride;

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1921: Type Ty = Dest->getType();
Same "const Type Ty = ..." comment as above.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.cpp:233: DestTy, Traits::getGprForType(DestTy,
Traits::RegisterSet::Reg_al));
I don't really like this Reg_al trick.

At the very least, leave a TODO to fix things.

Ideally, i1/i8/16 here would be a fatal error because it violates the PNaCl ABI.
 Consequently, helpers __Sz_bitcast_8xi1_i8() and __Sz_bitcast_16xi1_i16() would
be changed to return i32, and the bitcast lowering code would add a trunc after
the helper call to make the types work out.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.cpp:374: Traits::getGprForType(Src0Ty,
Traits::RegisterSet::Reg_eax));
Similar comment to above.  The lit tests that define functions with i1/i8/i16
return values are broken and ought to be fixed.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632Traits.h:375: assert(Ty == IceType_i8 || Ty ==
IceType_i1);
assert(isByteSizedType(Ty));

(looks like there are a few other places in the code base that could use this
change...)

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
types, not i8 or i16.
i32 type

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/jump_encodings.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
tests_lit/assembler/x86/jump_encodings.ll:143: ; Note: forward branches for
non-local labels in subzero currently use the fully
capitalize Subzero

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/opcode_register_encodings.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
tests_lit/assembler/x86/opcode_register_encodings.ll:164: ; CHECK: mov
eax,{{(DWORD PTR )?}}gs:0x0
Mostly just out of curiosity - was there a reason that the instruction bytes
needed to be removed?

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/nonsfi.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/nonsfi.ll:16: ; NONSFI: call {{.*}} {{(R_386_PC32
testCallRegular)?}}
This regex is a little silly.

    R_386_PC32 testCallRegular.  Or not.  I'm a regex, not a cop.

One possibility is to add a '$' to the regex.

Probably better would be to call a different function, maybe @testLoadBasic(). 
It was originally done as a self-call purely for convenience.

[sehr, 2016-01-11 21:49:48]

Thanks for the review.  I think I've addressed your request.  PTAL.

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone#ne...
Makefile.standalone:485: # Run spec2k for x86-32.
On 2016/01/10 03:08:55, stichnot wrote:
> Can you fix this comment?  It's actually running lit tests, cross tests, unit
> tests, and spec2k/x86-32.

Done.

https://codereview.chromium.org/1531623007/diff/160001/Makefile.standalone#ne...
Makefile.standalone:496: check-xtest FORCEASM=1
On 2016/01/10 03:08:55, stichnot wrote:
> Can you do this with a single sub-command?
> 
> +make -f Makefile.standalone FORCEASM=1 check-lit check-xtest

Done.

https://codereview.chromium.org/1531623007/diff/160001/pydir/crosstest_genera...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1531623007/diff/160001/pydir/crosstest_genera...
pydir/crosstest_generator.py:111: help='File type for compilation {asm, iasm,
obj}.')
On 2016/01/10 03:08:55, stichnot wrote:
> I suggest documenting this something like "...file type override..."

I'm not sure I totally got what you're saying, but have attempted.

https://codereview.chromium.org/1531623007/diff/160001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/160001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:44: help='output file to write')
On 2016/01/10 03:08:55, stichnot wrote:
> capitalize Output

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:826: Type DestTy = this->getDest()->getType();
On 2016/01/10 03:08:55, stichnot wrote:
> How about this:
> 
>   const Type DestTy = ArithmeticTypeOverride == IceType_void ?
> this->getDest()->getType() : ArithmeticTypeOverride;

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1921: Type Ty = Dest->getType();
On 2016/01/10 03:08:55, stichnot wrote:
> Same "const Type Ty = ..." comment as above.

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.cpp:233: DestTy, Traits::getGprForType(DestTy,
Traits::RegisterSet::Reg_al));
On 2016/01/10 03:08:56, stichnot wrote:
> I don't really like this Reg_al trick.
> 
> At the very least, leave a TODO to fix things.
> 
> Ideally, i1/i8/16 here would be a fatal error because it violates the PNaCl
ABI.
>  Consequently, helpers __Sz_bitcast_8xi1_i8() and __Sz_bitcast_16xi1_i16()
would
> be changed to return i32, and the bitcast lowering code would add a trunc
after
> the helper call to make the types work out.

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632.cpp:374: Traits::getGprForType(Src0Ty,
Traits::RegisterSet::Reg_eax));
On 2016/01/10 03:08:56, stichnot wrote:
> Similar comment to above.  The lit tests that define functions with i1/i8/i16
> return values are broken and ought to be fixed.

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632Traits.h:375: assert(Ty == IceType_i8 || Ty ==
IceType_i1);
On 2016/01/10 03:08:56, stichnot wrote:
> assert(isByteSizedType(Ty));
> 
> (looks like there are a few other places in the code base that could use this
> change...)

Done.

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/160001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
types, not i8 or i16.
On 2016/01/10 03:08:56, stichnot wrote:
> i32 type

Done.

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/jump_encodings.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
tests_lit/assembler/x86/jump_encodings.ll:143: ; Note: forward branches for
non-local labels in subzero currently use the fully
On 2016/01/10 03:08:56, stichnot wrote:
> capitalize Subzero

Done.

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
File tests_lit/assembler/x86/opcode_register_encodings.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/assembler/x8...
tests_lit/assembler/x86/opcode_register_encodings.ll:164: ; CHECK: mov
eax,{{(DWORD PTR )?}}gs:0x0
On 2016/01/10 03:08:56, stichnot wrote:
> Mostly just out of curiosity - was there a reason that the instruction bytes
> needed to be removed?

There are two totally different opcodes for moving to eax.  One the (shorter, no
modrm) special case, and one the generic.  The byte sequences are hence,
different.  Guess I could leave the immediate part, but I found that confusing.

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/nonsfi.ll (right):

https://codereview.chromium.org/1531623007/diff/160001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/nonsfi.ll:16: ; NONSFI: call {{.*}} {{(R_386_PC32
testCallRegular)?}}
On 2016/01/10 03:08:56, stichnot wrote:
> This regex is a little silly.
> 
>     R_386_PC32 testCallRegular.  Or not.  I'm a regex, not a cop.
> 
> One possibility is to add a '$' to the regex.
> 
> Probably better would be to call a different function, maybe @testLoadBasic().

> It was originally done as a self-call purely for convenience.

Done.

[Jim Stichnoth, 2016-01-12 14:54:03]

https://codereview.chromium.org/1531623007/diff/180001/pydir/crosstest_genera...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1531623007/diff/180001/pydir/crosstest_genera...
pydir/crosstest_generator.py:111: help='File type override, set to {asm, iasm,
obj}.')
Bikeshed: File type override, one of {asm,iasm,obj}.

https://codereview.chromium.org/1531623007/diff/180001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/180001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:161: if j == '-o':
This confused me for a bit.  I think it would help to add a comment that the
'-o' refers to the pnacl-sz option (as opposed to this script's -o option, which
was my initial misunderstanding).

Or maybe change the error message:

Output file should be set using:
  {sys.argv[0]} --output <outfile> ... --args ...
instead of:
  {sys.argv[0]} ... --args -o <outfile> ...

Also, it occurs to me that both "-o foo" and "-o=foo" are valid, also the "--o"
forms...

Along those lines (and this probably deserves just a TODO for now), this same
pnacl-sz arg checking could be very helpful for --filetype, --target, --sandbox,
and the positional arg (for which --input should be used instead).

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:826: Type DestTy = ArithmeticTypeOverride == IceType_void
const

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1920: Type Ty = ArithmeticTypeOverride == IceType_void ?
Dest->getType()
const

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
type, not i8 or i16.
What's the story with i64 under x86-64?  Ask John, he just had to do something
in this area for x86-64 sandboxing.

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5690: // Sometimes we need to convert from i8
or i16 type to i32 to maintain ABI
This is a little confusing because we're actually converting from i32 to i8/i16.
 How about something like this:

The PNaCl ABI disallows i8/i16 return types, so truncate the helper call result
to the appropriate type as necessary.

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5692: if (CallDest != Dest)
I think it would be marginally more future-proof if the condition were
"CallDest->getType() != Dest->getType()".

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/loop-nest-depth.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/loop-nest-depth.ll:6: ; RUN: %p2i --filetype=asm -i %s
--args -O2 --verbose=loop \
Add a comment as to why it's filetype=asm?

I think it's because you want to preserve and check the dump output, so maybe
you could change this to do "--args ... -log=%t" and then "FileCheck %t" ?  Then
keep the original filetype=obj etc.

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/nonsfi.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/nonsfi.ll:13: call i32 @testLoadBasic()
It would be helpful to add a comment here explaining the pattern here:

Make a call to a *different* function, plus use -ffunction-sections, to force an
appropriately-named relocation.

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/square.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/square.ll:36: define internal i32 @Square_i16(i16 %a) {
You should also change the i16 arg to be i32 (or leave it for Karl to fix).

(Here, below, and in other .ll files.)

[sehr, 2016-01-12 19:01:19]

I changed the parameter types requested by Karl.  PTAL to both of you :-).

https://codereview.chromium.org/1531623007/diff/180001/pydir/crosstest_genera...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1531623007/diff/180001/pydir/crosstest_genera...
pydir/crosstest_generator.py:111: help='File type override, set to {asm, iasm,
obj}.')
On 2016/01/12 14:54:02, stichnot wrote:
> Bikeshed: File type override, one of {asm,iasm,obj}.

Done.

https://codereview.chromium.org/1531623007/diff/180001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/180001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:161: if j == '-o':
On 2016/01/12 14:54:02, stichnot wrote:
> This confused me for a bit.  I think it would help to add a comment that the
> '-o' refers to the pnacl-sz option (as opposed to this script's -o option,
which
> was my initial misunderstanding).
> 
> Or maybe change the error message:
> 
> Output file should be set using:
>   {sys.argv[0]} --output <outfile> ... --args ...
> instead of:
>   {sys.argv[0]} ... --args -o <outfile> ...
> 
> Also, it occurs to me that both "-o foo" and "-o=foo" are valid, also the
"--o"
> forms...
> 
> Along those lines (and this probably deserves just a TODO for now), this same
> pnacl-sz arg checking could be very helpful for --filetype, --target,
--sandbox,
> and the positional arg (for which --input should be used instead).

I ventured a bit into python regexes.  At your mercy there, but otherwise, done.

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:826: Type DestTy = ArithmeticTypeOverride == IceType_void
On 2016/01/12 14:54:02, stichnot wrote:
> const

Done.

https://codereview.chromium.org/1531623007/diff/180001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1920: Type Ty = ArithmeticTypeOverride == IceType_void ?
Dest->getType()
On 2016/01/12 14:54:02, stichnot wrote:
> const

Done.

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
type, not i8 or i16.
On 2016/01/12 14:54:02, stichnot wrote:
> What's the story with i64 under x86-64?  Ask John, he just had to do something
> in this area for x86-64 sandboxing.

Not supported either.  I changed the comment.

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5690: // Sometimes we need to convert from i8
or i16 type to i32 to maintain ABI
On 2016/01/12 14:54:02, stichnot wrote:
> This is a little confusing because we're actually converting from i32 to
i8/i16.
>  How about something like this:
> 
> The PNaCl ABI disallows i8/i16 return types, so truncate the helper call
result
> to the appropriate type as necessary.

Done.

https://codereview.chromium.org/1531623007/diff/180001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:5692: if (CallDest != Dest)
On 2016/01/12 14:54:02, stichnot wrote:
> I think it would be marginally more future-proof if the condition were
> "CallDest->getType() != Dest->getType()".

Done.

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/loop-nest-depth.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/loop-nest-depth.ll:6: ; RUN: %p2i --filetype=asm -i %s
--args -O2 --verbose=loop \
On 2016/01/12 14:54:03, stichnot wrote:
> Add a comment as to why it's filetype=asm?
> 
> I think it's because you want to preserve and check the dump output, so maybe
> you could change this to do "--args ... -log=%t" and then "FileCheck %t" ? 
Then
> keep the original filetype=obj etc.

Done.

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/nonsfi.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/nonsfi.ll:13: call i32 @testLoadBasic()
On 2016/01/12 14:54:03, stichnot wrote:
> It would be helpful to add a comment here explaining the pattern here:
> 
> Make a call to a *different* function, plus use -ffunction-sections, to force
an
> appropriately-named relocation.

Done.

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/square.ll (right):

https://codereview.chromium.org/1531623007/diff/180001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/square.ll:36: define internal i32 @Square_i16(i16 %a) {
On 2016/01/12 14:54:03, stichnot wrote:
> You should also change the i16 arg to be i32 (or leave it for Karl to fix).
> 
> (Here, below, and in other .ll files.)

Done.

[Jim Stichnoth, 2016-01-13 00:36:44]

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:158: # Redirecting the output file needs to be done
through the script because
This comment should probably be moved closer to the "-o" block.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:160: for i, j in enumerate(args.args):
You're not using "i", right?  I think the typical pattern for this is something
like:

  for _, arg in enumerate(args.args):

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:161: if re.search('^-?-o(=.+)?', j):
I think I would just use a single big regex here, and if it matches, just report
that the arg should be used before --args instead of after, and don't bother
trying to translate between pnacl-sz args and this script's args.

In bash regex, it would be something like:
  {-,--}{o,output,filetype,target,sandbox}{=,}

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:162: scriptname = os.path.basename(sys.argv[0])
Hoist this out of the loop, and remove duplication below.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:175: elif re.search('^--target(=.+)?', j):
Add a similar check for --sandbox

https://codereview.chromium.org/1531623007/diff/240001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/240001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
type, not i8 or i16 (or i64 on x86-64).
This suggests to me that i64 lea optimization does work on x86-32?

How about:

Lea optimization only works for i32 type, not i1/i8/i16/i64.

https://codereview.chromium.org/1531623007/diff/240001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/loop-nest-depth.ll (right):

https://codereview.chromium.org/1531623007/diff/240001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/loop-nest-depth.ll:5: ; Single threaded so that the
dumps used for checking happen in order
End this sentence with a period.

[sehr, 2016-01-13 18:15:35]

Thanks again.  PTAL.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:158: # Redirecting the output file needs to be done
through the script because
On 2016/01/13 00:36:44, stichnot wrote:
> This comment should probably be moved closer to the "-o" block.

Done.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:160: for i, j in enumerate(args.args):
On 2016/01/13 00:36:44, stichnot wrote:
> You're not using "i", right?  I think the typical pattern for this is
something
> like:
> 
>   for _, arg in enumerate(args.args):

Done.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:161: if re.search('^-?-o(=.+)?', j):
On 2016/01/13 00:36:44, stichnot wrote:
> I think I would just use a single big regex here, and if it matches, just
report
> that the arg should be used before --args instead of after, and don't bother
> trying to translate between pnacl-sz args and this script's args.
> 
> In bash regex, it would be something like:
>   {-,--}{o,output,filetype,target,sandbox}{=,}

Done.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:162: scriptname = os.path.basename(sys.argv[0])
On 2016/01/13 00:36:44, stichnot wrote:
> Hoist this out of the loop, and remove duplication below.

Done.

https://codereview.chromium.org/1531623007/diff/240001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:175: elif re.search('^--target(=.+)?', j):
On 2016/01/13 00:36:44, stichnot wrote:
> Add a similar check for --sandbox

Done.

https://codereview.chromium.org/1531623007/diff/240001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1531623007/diff/240001/src/IceTargetLoweringX...
src/IceTargetLoweringX86BaseImpl.h:1138: // Lea optimization only works for i32
type, not i8 or i16 (or i64 on x86-64).
On 2016/01/13 00:36:44, stichnot wrote:
> This suggests to me that i64 lea optimization does work on x86-32?
> 
> How about:
> 
> Lea optimization only works for i32 type, not i1/i8/i16/i64.

Done.

https://codereview.chromium.org/1531623007/diff/240001/tests_lit/llvm2ice_tes...
File tests_lit/llvm2ice_tests/loop-nest-depth.ll (right):

https://codereview.chromium.org/1531623007/diff/240001/tests_lit/llvm2ice_tes...
tests_lit/llvm2ice_tests/loop-nest-depth.ll:5: ; Single threaded so that the
dumps used for checking happen in order
On 2016/01/13 00:36:44, stichnot wrote:
> End this sentence with a period.

Done.

[Jim Stichnoth, 2016-01-13 22:00:44]

otherwise lgtm

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:159: for _, j in enumerate(args.args):
'j' looks kind of odd as a variable name here.  How about 'arg'?

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:164: if
re.search('^-?-(o|filetype|target|sandbox)(=.+)?$', j):
add 'output' as well because pnacl-sz -o and --output are the same

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:168: preferred_option = j
preferred_option = '--output' if re.search('^-?-o', j) else j

also, may want to check for the '-o=' form.

[sehr, 2016-01-13 22:17:41]

Description was changed from

==========
Add option to force filetype=asm for testing

Also turn it on for presubmit testing.

BUG=
==========

to

==========
Add option to force filetype=asm for testing

Also turn it on for presubmit testing.

BUG=
R=stichnot@chromium.org

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

[sehr, 2016-01-13 22:17:42]

Committed patchset #15 (id:280001) manually as
b19d39cc5fa8dc60c678c2507af02147184f168f (presubmit successful).

[sehr, 2016-01-13 22:19:39]

Committed.

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:159: for _, j in enumerate(args.args):
On 2016/01/13 22:00:44, stichnot wrote:
> 'j' looks kind of odd as a variable name here.  How about 'arg'?

Done.

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:164: if
re.search('^-?-(o|filetype|target|sandbox)(=.+)?$', j):
On 2016/01/13 22:00:44, stichnot wrote:
> add 'output' as well because pnacl-sz -o and --output are the same

Done.

https://codereview.chromium.org/1531623007/diff/280001/pydir/run-pnacl-sz.py#...
pydir/run-pnacl-sz.py:168: preferred_option = j
On 2016/01/13 22:00:44, stichnot wrote:
> preferred_option = '--output' if re.search('^-?-o', j) else j
> 
> also, may want to check for the '-o=' form.

Done.  Thanks, I should have thought of that...