Add a few Subzero intrinsics (not the atomic ones yet).

Add a few Subzero intrinsics (not the atomic ones yet).

Handle:
* mem{cpy,move,set} (without optimizations for known lengths)
* nacl.read.tp
* setjmp, longjmp
* trap

Mostly see if the dispatching/organization is okay.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3882
R=stichnot@chromium.org

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

[JF, 2014-06-10 03:50:41]

This CL also seems like the right place to add something about side-effects,
what touches memory, etc... Those will have to be respected by Subzero's
optimizations, and knowing what each function does is pretty important (and
intrinsic semantics are perfectly known).

https://codereview.chromium.org/321993002/diff/50001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:98: BuildIntrinsicMap(&IntrinsicInfos);
Why isn't think just IntrinsicMap's default constructor instead?

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode303
src/IceInst.h:303: InstKind Kind=Inst::Call)
I think it's better to avoid default arguments in general. It seems cleaner to
pass Inst::Call in InstCall::create above.

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode403
src/IceInst.h:403: // Call to an intrinsic funciton.  The call target is
captured as getSrc(0),
s/funciton/function/

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:96: SizeT getSegmentRegister() const { return SegmentReg; }
It seems weird to have SegmentReg as a SizeT. Shouldn't it have a separate
typedef?

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:33: const struct IceIntrinsicsEntry_ {
This should be in an anonymous namespace.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:75: { {Trap, true}, "trap" }
Do you need the function signatures too? The lowering code could use that
information to be more generic.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h
File src/IceIntrinsics.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h#newc...
src/IceIntrinsics.h:57: };
I'd make this a 31-bit field followed by a 1-bit field. It'll get padded to that
size anyways.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h#newc...
src/IceIntrinsics.h:63: typedef std::map<IceString, IntrinsicInfo> IntrinsicMap;
It would be better to turn all of this into a class that has the map inside
itself. It can also contain the enum, so that its names are better scoped.

[Jim Stichnoth, 2014-06-10 22:58:29]

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode423
src/IceInst.h:423: HasSideEffects = Info.HasSideEffects;
I think I'd be more comfortable if the InstCall ctor took a HasSideEffects
argument.  Otherwise, you have the Inst ctor setting it to false, then InstCall
ctor setting it to true, and finally InstIntrinsicCall ctor possibly setting it
back to false.

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:87: class OperandX8632MemOffSeg : public OperandX8632 {
Instead of creating this new class, couldn't we just add a segment register to
OperandX8632Mem?  That way the segment register could be used for all addressing
modes, not just immediate addresses.

I would probably use a separate OperandX8632Mem ctor with an explicit segment
register, and the original ctor would be for the default segment.

Address mode optimization would have to preserve the original segment register. 
Legalization would have to do the same.

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:96: SizeT getSegmentRegister() const { return SegmentReg; }
On 2014/06/10 03:50:41, JF wrote:
> It seems weird to have SegmentReg as a SizeT. Shouldn't it have a separate
> typedef?

The regular registers are using int32_t, but I wonder if this should just be
using enum SegmentRegisters.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:75: { {Trap, true}, "trap" }
On 2014/06/10 03:50:41, JF wrote:
> Do you need the function signatures too? The lowering code could use that
> information to be more generic.

I'm not sure what in particular the lowering code could do.  It could validate
the argument types, but that may be better done by the parser.

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1784: Call->addArg(Instr->getSrc(1));
Change all the Instr->getSrc(N) calls to Instr->getArg(N-1).

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1821: Dest->setWeightInfinite();
This will force Dest to have a register for the entire function, which may not
be desirable.  The right thing to do is to assign to an infinite-weight
temporary, then assign that temporary to Dest.

  Variable *T = NULL;
  _mov(T, Src);
  _mov(Dest, T);

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp#newcode533
src/llvm2ice.cpp:533: // Check if this direct call is to an Intrinsic (starts
with "llvm.")
I hate that std::string doesn't have a simple startsWith() method.  Can this be
an easier-to-read test like Name.substr(0,5)=="llvm." ?

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:8: ; Don't actually do the
szdiff test: Subzero treats pointers as i32,
Could probably add "i8 \*" to ignore_pattern in szdiff.py.

[jvoung (off chromium), 2014-06-12 05:48:30]

Thanks! Addressed some of the comments and will get to the rest soon. Also need
to debug the runtime crash in the new crosstest =)

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode303
src/IceInst.h:303: InstKind Kind=Inst::Call)
On 2014/06/10 03:50:41, JF wrote:
> I think it's better to avoid default arguments in general. It seems cleaner to
> pass Inst::Call in InstCall::create above.

Done.

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode403
src/IceInst.h:403: // Call to an intrinsic funciton.  The call target is
captured as getSrc(0),
On 2014/06/10 03:50:41, JF wrote:
> s/funciton/function/

Done.

https://codereview.chromium.org/321993002/diff/50001/src/IceInst.h#newcode423
src/IceInst.h:423: HasSideEffects = Info.HasSideEffects;
On 2014/06/10 22:58:28, stichnot wrote:
> I think I'd be more comfortable if the InstCall ctor took a HasSideEffects
> argument.  Otherwise, you have the Inst ctor setting it to false, then
InstCall
> ctor setting it to true, and finally InstIntrinsicCall ctor possibly setting
it
> back to false.

Done.

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:87: class OperandX8632MemOffSeg : public OperandX8632 {
On 2014/06/10 22:58:28, stichnot wrote:
> Instead of creating this new class, couldn't we just add a segment register to
> OperandX8632Mem?  That way the segment register could be used for all
addressing
> modes, not just immediate addresses.
> 
> I would probably use a separate OperandX8632Mem ctor with an explicit segment
> register, and the original ctor would be for the default segment.
> 
> Address mode optimization would have to preserve the original segment
register. 
> Legalization would have to do the same.

Done.

The initial concern was that we would be wasting some space for OperandX8632Mem
when the common case is a default segment register. Added to OperandX8632Mem,
but sharing some bits w/ Shift.

I kept it to a single ctor though, since the ctor itself is non-trivial, and
leveraged the create() instead. The create() already had a bunch of default
args... preference for (a) continuing to use the default args or (b) splitting
it up? Either way, if the function name is still called "create" in all
instances and we rely on overloading, it seems likely to forget to propagate
some of the arguments (e.g., the segment register) during legalize(), or
loOperand().

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:96: SizeT getSegmentRegister() const { return SegmentReg; }
On 2014/06/10 22:58:28, stichnot wrote:
> On 2014/06/10 03:50:41, JF wrote:
> > It seems weird to have SegmentReg as a SizeT. Shouldn't it have a separate
> > typedef?
> 
> The regular registers are using int32_t, but I wonder if this should just be
> using enum SegmentRegisters.

Moved to using the enum.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:33: const struct IceIntrinsicsEntry_ {
On 2014/06/10 03:50:41, JF wrote:
> This should be in an anonymous namespace.

Done.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:75: { {Trap, true}, "trap" }
On 2014/06/10 22:58:28, stichnot wrote:
> On 2014/06/10 03:50:41, JF wrote:
> > Do you need the function signatures too? The lowering code could use that
> > information to be more generic.
> 
> I'm not sure what in particular the lowering code could do.  It could validate
> the argument types, but that may be better done by the parser.


Yeah, I think type signatures would be most useful for the bitcode reader/parser
to do some sanity checking.

Otherwise, the InstCall instruction that you create after the parsing phase
should have the type information already.

I'll work on adding the expected signature and add some checks to the parser
(not done yet).

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1784: Call->addArg(Instr->getSrc(1));
On 2014/06/10 22:58:28, stichnot wrote:
> Change all the Instr->getSrc(N) calls to Instr->getArg(N-1).

Done.

Also adding a crosstest runnable test. Currently getting a segfault before I
even hit the memcpy though. Maybe stack alignment problem? Will debug tomorrow.

https://codereview.chromium.org/321993002/diff/50001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1821: Dest->setWeightInfinite();
On 2014/06/10 22:58:28, stichnot wrote:
> This will force Dest to have a register for the entire function, which may not
> be desirable.  The right thing to do is to assign to an infinite-weight
> temporary, then assign that temporary to Dest.
> 
>   Variable *T = NULL;
>   _mov(T, Src);
>   _mov(Dest, T);

Oops yeah otherwise it could potentially have a pretty long lifetime -- Done.

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp#newcode533
src/llvm2ice.cpp:533: // Check if this direct call is to an Intrinsic (starts
with "llvm.")
On 2014/06/10 22:58:28, stichnot wrote:
> I hate that std::string doesn't have a simple startsWith() method.  Can this
be
> an easier-to-read test like Name.substr(0,5)=="llvm." ?

Done -- at some point we might want to just some string utilities -- E.g.,
LLVM's StringRef has startswith(), but what you suggest works =)

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:8: ; Don't actually do the
szdiff test: Subzero treats pointers as i32,
On 2014/06/10 22:58:28, stichnot wrote:
> Could probably add "i8 \*" to ignore_pattern in szdiff.py.

Ignoring "@llvm." lines instead of i8\* -- Unfortunately, there's also i16*,
i32*, i64* when we include the atomic intrinsics, and that ignoring all pointer
types would affect the loads and stores too.

Also added line wrap normalization, and reordered some of the instructions to
make the --pnacl version's cast-eliding/cast-reinsertion happy (potentially
reinserts in a different order from original order). The cast-reinsertion also
makes the variable names different, so manually tweaked that for now...

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1811: Call->addArg(Instr->getSrc(2));
Should this code be responsible for "widening" the type to a stack slot size? W/
intrinsics, we can violate the rule that PNaCl function parameters are at least
i32 =(

I believe I ran into a .s file attempting to "push byte ptr [...]" which isn't
allowed. Will have to check again.

[JF, 2014-06-12 15:43:02]

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:75: { {Trap, true}, "trap" }
On 2014/06/12 05:48:30, jvoung wrote:
> On 2014/06/10 22:58:28, stichnot wrote:
> > On 2014/06/10 03:50:41, JF wrote:
> > > Do you need the function signatures too? The lowering code could use that
> > > information to be more generic.
> > 
> > I'm not sure what in particular the lowering code could do.  It could
validate
> > the argument types, but that may be better done by the parser.
> 
> 
> Yeah, I think type signatures would be most useful for the bitcode
reader/parser
> to do some sanity checking.
> 
> Otherwise, the InstCall instruction that you create after the parsing phase
> should have the type information already.
> 
> I'll work on adding the expected signature and add some checks to the parser
> (not done yet).

I was thinking of something similar to llvm/IR/NaClAtomicIntrinsics.h, where you
define the signatures in one place and can iterate over them somewhere else.

[Jim Stichnoth, 2014-06-12 17:04:44]

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1811: Call->addArg(Instr->getSrc(2));
On 2014/06/12 05:48:30, jvoung wrote:
> Should this code be responsible for "widening" the type to a stack slot size?
W/
> intrinsics, we can violate the rule that PNaCl function parameters are at
least
> i32 =(
> 
> I believe I ran into a .s file attempting to "push byte ptr [...]" which isn't
> allowed. Will have to check again.

The Subzero lowering code assumes the simplification pass that widens i1/i8/i16
function args and return values to i32.  If this hasn't happened for some
intrinsics, then yeah, we'll probably end up with illegal push or other
instructions.

It would probably be best to fix this up on a per-arg basis in lowerCall(), and
even better if there's a clean way to fix-up for specific intrinsics and assert
for everything else.

[Jim Stichnoth, 2014-06-12 20:52:08]

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceInstX8632.h#newco...
src/IceInstX8632.h:87: class OperandX8632MemOffSeg : public OperandX8632 {
On 2014/06/12 05:48:30, jvoung wrote:
> On 2014/06/10 22:58:28, stichnot wrote:
> > Instead of creating this new class, couldn't we just add a segment register
to
> > OperandX8632Mem?  That way the segment register could be used for all
> addressing
> > modes, not just immediate addresses.
> > 
> > I would probably use a separate OperandX8632Mem ctor with an explicit
segment
> > register, and the original ctor would be for the default segment.
> > 
> > Address mode optimization would have to preserve the original segment
> register. 
> > Legalization would have to do the same.
> 
> Done.
> 
> The initial concern was that we would be wasting some space for
OperandX8632Mem
> when the common case is a default segment register. Added to OperandX8632Mem,
> but sharing some bits w/ Shift.
> 
> I kept it to a single ctor though, since the ctor itself is non-trivial, and
> leveraged the create() instead. The create() already had a bunch of default
> args... preference for (a) continuing to use the default args or (b) splitting
> it up? Either way, if the function name is still called "create" in all
> instances and we rely on overloading, it seems likely to forget to propagate
> some of the arguments (e.g., the segment register) during legalize(), or
> loOperand().

You're right, I should have been looking at create() not the ctor.  The current
implementation looks good to me.

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp#newcode533
src/llvm2ice.cpp:533: // Check if this direct call is to an Intrinsic (starts
with "llvm.")
On 2014/06/12 05:48:30, jvoung wrote:
> On 2014/06/10 22:58:28, stichnot wrote:
> > I hate that std::string doesn't have a simple startsWith() method.  Can this
> be
> > an easier-to-read test like Name.substr(0,5)=="llvm." ?
> 
> Done -- at some point we might want to just some string utilities -- E.g.,
> LLVM's StringRef has startswith(), but what you suggest works =)

Sorry, I used the constant 5 in my suggestion just to be concise, but it would
be a lot better to be more symbolic, e.g.

static const char *LlvmPrefix = "llvm.";
const size_t LlvmPrefixLen = strlen(LlvmPrefix);
... Name.substr(0, LlvmPrefixLen) == LlvmPrefix ...


The strlen() call ought to get computed at compile time, but not a big deal if
it doesn't.

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll (right):

https://codereview.chromium.org/321993002/diff/50001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll:8: ; Don't actually do the
szdiff test: Subzero treats pointers as i32,
On 2014/06/12 05:48:30, jvoung wrote:
> On 2014/06/10 22:58:28, stichnot wrote:
> > Could probably add "i8 \*" to ignore_pattern in szdiff.py.
> 
> Ignoring mailto:"@llvm." lines instead of i8\* -- Unfortunately, there's also
i16*,
> i32*, i64* when we include the atomic intrinsics, and that ignoring all
pointer
> types would affect the loads and stores too.
> 
> Also added line wrap normalization, and reordered some of the instructions to
> make the --pnacl version's cast-eliding/cast-reinsertion happy (potentially
> reinserts in a different order from original order). The cast-reinsertion also
> makes the variable names different, so manually tweaked that for now...

Ouch.  At some point we may want to just retire szdiff.py...

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin.c
File crosstest/mem_intrin.c (right):

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin.c#...
crosstest/mem_intrin.c:33: * when you stack allocate two buffers. So, instead of
allocating
Hmm.  I guess we need some better alloca tests...

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin_ma...
File crosstest/mem_intrin_main.c (right):

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin_ma...
crosstest/mem_intrin_main.c:18: #define do_test(test_func) \
Do you think this test will be getting much bigger?  If it does, it would be
worthwhile making it a C++ test instead of C.  That would make templates
available, and makes declarations a bit simpler (#include a header twice - once
inside a namespace Subzero_ {} block and once at the top level).

The only downside of C++ cross tests that I'm aware of is that the name mangling
is a pain when the test includes a manually written .ll file.

https://codereview.chromium.org/321993002/diff/130001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInst.h#newcode295
src/IceInst.h:295: InstCall(Func, NumArgs, Dest, CallTarget, true, Inst::Call);
I don't know if I've been 100% consistent with this, but I think it's clearer to
do like this:

const bool HasSideEffects = true;
return new InstCall(..., HasSideEffects, ...);

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:860: Offset->emit(Func);
emit --> dump

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:864: Offset->emit(Func);
emit --> dump

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.def#ne...
src/IceInstX8632.def:44: X(Reg_CS, "cs")           \
Maybe we could use a prefix other than "Reg_" for the enum values?  Just to
convey the fact that segment registers are a very different register class than
the other Reg_* registers, e.g. segment registers can't be used for register
allocation.

I'd suggest Sreg_ or SegReg_, and be consistent with SegReg_NUM (which could
mean renaming this to Sreg_NUM).

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:29: // OperandX8632Mem, and VariableSplit.
Remove the comma. :)

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

https://codereview.chromium.org/321993002/diff/130001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2041: // At the ICE level, load instructions
should not use the
ICE includes both high-level (close to bitcode) and low-level (target
instructions.  You probably want to say "High-level ICE load instructions...",
or my favorite, "Vanilla ICE".  Here and below.

https://codereview.chromium.org/321993002/diff/130001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2045: OperandX8632Mem::SegmentRegisters
SegmentReg =
Mark this const?  (here and below)

https://codereview.chromium.org/321993002/diff/130001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/321993002/diff/130001/szdiff.py#newcode74
szdiff.py:74: ' i.*\* @llvm\.',          # intrinsic calls w/ pointer ret
Maybe this should be more precise like:
 i\d+ \* @llvm\.

[jvoung (off chromium), 2014-06-16 20:51:59]

https://codereview.chromium.org/321993002/diff/50001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:98: BuildIntrinsicMap(&IntrinsicInfos);
On 2014/06/10 03:50:41, JF wrote:
> Why isn't think just IntrinsicMap's default constructor instead?

Done.

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h
File src/IceIntrinsics.h (right):

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h#newc...
src/IceIntrinsics.h:57: };
On 2014/06/10 03:50:41, JF wrote:
> I'd make this a 31-bit field followed by a 1-bit field. It'll get padded to
that
> size anyways.

Done.

Though I would like to enforce that IntrinsicID only needs a few bits (say 16
bits), so that the attributes can grow beyond "HasSideEffects".

https://codereview.chromium.org/321993002/diff/50001/src/IceIntrinsics.h#newc...
src/IceIntrinsics.h:63: typedef std::map<IceString, IntrinsicInfo> IntrinsicMap;
On 2014/06/10 03:50:41, JF wrote:
> It would be better to turn all of this into a class that has the map inside
> itself. It can also contain the enum, so that its names are better scoped.

Done.

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/321993002/diff/50001/src/llvm2ice.cpp#newcode533
src/llvm2ice.cpp:533: // Check if this direct call is to an Intrinsic (starts
with "llvm.")
On 2014/06/12 20:52:06, stichnot wrote:
> On 2014/06/12 05:48:30, jvoung wrote:
> > On 2014/06/10 22:58:28, stichnot wrote:
> > > I hate that std::string doesn't have a simple startsWith() method.  Can
this
> > be
> > > an easier-to-read test like Name.substr(0,5)=="llvm." ?
> > 
> > Done -- at some point we might want to just some string utilities -- E.g.,
> > LLVM's StringRef has startswith(), but what you suggest works =)
> 
> Sorry, I used the constant 5 in my suggestion just to be concise, but it would
> be a lot better to be more symbolic, e.g.
> 
> static const char *LlvmPrefix = "llvm.";
> const size_t LlvmPrefixLen = strlen(LlvmPrefix);
> ... Name.substr(0, LlvmPrefixLen) == LlvmPrefix ...
> 
> 
> The strlen() call ought to get computed at compile time, but not a big deal if
> it doesn't.

Done.

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1811: Call->addArg(Instr->getSrc(2));
On 2014/06/12 17:04:44, stichnot wrote:
> On 2014/06/12 05:48:30, jvoung wrote:
> > Should this code be responsible for "widening" the type to a stack slot
size?
> W/
> > intrinsics, we can violate the rule that PNaCl function parameters are at
> least
> > i32 =(
> > 
> > I believe I ran into a .s file attempting to "push byte ptr [...]" which
isn't
> > allowed. Will have to check again.
> 
> The Subzero lowering code assumes the simplification pass that widens
i1/i8/i16
> function args and return values to i32.  If this hasn't happened for some
> intrinsics, then yeah, we'll probably end up with illegal push or other
> instructions.
> 
> It would probably be best to fix this up on a per-arg basis in lowerCall(),
and
> even better if there's a clean way to fix-up for specific intrinsics and
assert
> for everything else.


Added a movzx for this case.

Was going to add a debug assert in InstX8632Push::emit, but looks like some
tests currently don't handle that yet?

====

Assert from push::emit:

${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/cmp-opt.ll -o - |
llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -

<stdin>:27:7: error: invalid operand for instruction
        push    dl
                ^~
<stdin>:37:7: error: invalid operand for instruction
        push    bl
                ^~

=====

Assert from lowerCall:

${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/bool-opt.ll -o - |
llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -
<stdin>:16:7: error: invalid operand for instruction
        push    al
                ^~
	call	use

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin.c
File crosstest/mem_intrin.c (right):

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin.c#...
crosstest/mem_intrin.c:33: * when you stack allocate two buffers. So, instead of
allocating
On 2014/06/12 20:52:07, stichnot wrote:
> Hmm.  I guess we need some better alloca tests...

After the fix landed this works!
Removed workaround.

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin_ma...
File crosstest/mem_intrin_main.c (right):

https://codereview.chromium.org/321993002/diff/130001/crosstest/mem_intrin_ma...
crosstest/mem_intrin_main.c:18: #define do_test(test_func) \
On 2014/06/12 20:52:07, stichnot wrote:
> Do you think this test will be getting much bigger?  If it does, it would be
> worthwhile making it a C++ test instead of C.  That would make templates
> available, and makes declarations a bit simpler (#include a header twice -
once
> inside a namespace Subzero_ {} block and once at the top level).
> 
> The only downside of C++ cross tests that I'm aware of is that the name
mangling
> is a pain when the test includes a manually written .ll file.

Added some variable-length mem() calls, (vs the fixed size alloca buffers).

Converted to c++, but didn't really use it for much besides the namespace
Subzero_ #include trick.

One thing to watch out for is more complex name-mangling rules. E.g.,
compression / element substitution.

I don't know if we should make the Subzero name-mangling injection any more
complicated, though. Let me know if you disagree. I had one workaround (make a
param void* instead of uint8_t*).

====

Otherwise, I there is a test in scons for memcpy/memmove/memset, so I didn't go
and make this more complicated.

https://codereview.chromium.org/321993002/diff/130001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInst.h#newcode295
src/IceInst.h:295: InstCall(Func, NumArgs, Dest, CallTarget, true, Inst::Call);
On 2014/06/12 20:52:07, stichnot wrote:
> I don't know if I've been 100% consistent with this, but I think it's clearer
to
> do like this:
> 
> const bool HasSideEffects = true;
> return new InstCall(..., HasSideEffects, ...);

Yep that's more readable -- Done.

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:860: Offset->emit(Func);
On 2014/06/12 20:52:07, stichnot wrote:
> emit --> dump

Oops the dangers of copy paste. Done.

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:864: Offset->emit(Func);
On 2014/06/12 20:52:07, stichnot wrote:
> emit --> dump

Done.

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.def#ne...
src/IceInstX8632.def:44: X(Reg_CS, "cs")           \
On 2014/06/12 20:52:07, stichnot wrote:
> Maybe we could use a prefix other than "Reg_" for the enum values?  Just to
> convey the fact that segment registers are a very different register class
than
> the other Reg_* registers, e.g. segment registers can't be used for register
> allocation.
> 
> I'd suggest Sreg_ or SegReg_, and be consistent with SegReg_NUM (which could
> mean renaming this to Sreg_NUM).

Changing to SegReg_ (and matches the table's name SEG_REG)

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/321993002/diff/130001/src/IceInstX8632.h#newc...
src/IceInstX8632.h:29: // OperandX8632Mem, and VariableSplit.
On 2014/06/12 20:52:07, stichnot wrote:
> Remove the comma. :)

Done.

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

https://codereview.chromium.org/321993002/diff/130001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2041: // At the ICE level, load instructions
should not use the
On 2014/06/12 20:52:07, stichnot wrote:
> ICE includes both high-level (close to bitcode) and low-level (target
> instructions.  You probably want to say "High-level ICE load instructions...",
> or my favorite, "Vanilla ICE".  Here and below.

Done.

https://codereview.chromium.org/321993002/diff/130001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2045: OperandX8632Mem::SegmentRegisters
SegmentReg =
On 2014/06/12 20:52:07, stichnot wrote:
> Mark this const?  (here and below)

Done.

https://codereview.chromium.org/321993002/diff/130001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/321993002/diff/130001/szdiff.py#newcode74
szdiff.py:74: ' i.*\* @llvm\.',          # intrinsic calls w/ pointer ret
On 2014/06/12 20:52:07, stichnot wrote:
> Maybe this should be more precise like:
>  i\d+ \* @llvm\.

Done.

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh#n...
crosstest/runtests.sh:66: "${OUTDIR}"/mem_intrin_O${optlevel}
Should I put "test_" prefix?

Keep it sorted?

[Jim Stichnoth, 2014-06-16 23:05:16]

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1811: Call->addArg(Instr->getSrc(2));
On 2014/06/16 20:51:58, jvoung wrote:
> On 2014/06/12 17:04:44, stichnot wrote:
> > On 2014/06/12 05:48:30, jvoung wrote:
> > > Should this code be responsible for "widening" the type to a stack slot
> size?
> > W/
> > > intrinsics, we can violate the rule that PNaCl function parameters are at
> > least
> > > i32 =(
> > > 
> > > I believe I ran into a .s file attempting to "push byte ptr [...]" which
> isn't
> > > allowed. Will have to check again.
> > 
> > The Subzero lowering code assumes the simplification pass that widens
> i1/i8/i16
> > function args and return values to i32.  If this hasn't happened for some
> > intrinsics, then yeah, we'll probably end up with illegal push or other
> > instructions.
> > 
> > It would probably be best to fix this up on a per-arg basis in lowerCall(),
> and
> > even better if there's a clean way to fix-up for specific intrinsics and
> assert
> > for everything else.
> 
> 
> Added a movzx for this case.
> 
> Was going to add a debug assert in InstX8632Push::emit, but looks like some
> tests currently don't handle that yet?
> 
> ====
> 
> Assert from push::emit:
> 
> ${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
> ${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/cmp-opt.ll -o - |
> llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -
> 
> <stdin>:27:7: error: invalid operand for instruction
>         push    dl
>                 ^~
> <stdin>:37:7: error: invalid operand for instruction
>         push    bl
>                 ^~
> 
> =====
> 
> Assert from lowerCall:
> 
> ${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
> ${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/bool-opt.ll -o - |
> llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -
> <stdin>:16:7: error: invalid operand for instruction
>         push    al
>                 ^~
> 	call	use
> 

Hmm, looks like those two tests are not valid PNaCl bitcode since their use()
function is declared to take an i1 arg. :(

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh#n...
crosstest/runtests.sh:66: "${OUTDIR}"/mem_intrin_O${optlevel}
On 2014/06/16 20:51:59, jvoung wrote:
> Should I put "test_" prefix?
> 
> Keep it sorted?

I guess I have no real preference here (except to append with _Om1 / _O2).  And
there was no real consistency before. :)

https://codereview.chromium.org/321993002/diff/320001/crosstest/mem_intrin.h
File crosstest/mem_intrin.h (right):

https://codereview.chromium.org/321993002/diff/320001/crosstest/mem_intrin.h#...
crosstest/mem_intrin.h:6: #include <stdint.h> /* cstdint requires -std=c++0x or
higher */
Since mem_intrin.h is now being #included inside namespace Subzero_ {}, I don't
think you should be #including standard headers, right?  Or does C++ somehow pop
out of namespaces for these includes?

https://codereview.chromium.org/321993002/diff/320001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/320001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:38: , "nacl.atomic.cmpxchg." #NameSuffix                  
                    \
I don't think the # is necessary here, is it?  (here and below)

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

https://codereview.chromium.org/321993002/diff/320001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2174: // Vanilla ICE load instructions should not
use the segment registers,
load --> store

[jvoung (off chromium), 2014-06-17 17:36:19]

Thanks!

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/321993002/diff/90001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1811: Call->addArg(Instr->getSrc(2));
On 2014/06/16 23:05:15, stichnot wrote:
> On 2014/06/16 20:51:58, jvoung wrote:
> > On 2014/06/12 17:04:44, stichnot wrote:
> > > On 2014/06/12 05:48:30, jvoung wrote:
> > > > Should this code be responsible for "widening" the type to a stack slot
> > size?
> > > W/
> > > > intrinsics, we can violate the rule that PNaCl function parameters are
at
> > > least
> > > > i32 =(
> > > > 
> > > > I believe I ran into a .s file attempting to "push byte ptr [...]" which
> > isn't
> > > > allowed. Will have to check again.
> > > 
> > > The Subzero lowering code assumes the simplification pass that widens
> > i1/i8/i16
> > > function args and return values to i32.  If this hasn't happened for some
> > > intrinsics, then yeah, we'll probably end up with illegal push or other
> > > instructions.
> > > 
> > > It would probably be best to fix this up on a per-arg basis in
lowerCall(),
> > and
> > > even better if there's a clean way to fix-up for specific intrinsics and
> > assert
> > > for everything else.
> > 
> > 
> > Added a movzx for this case.
> > 
> > Was going to add a debug assert in InstX8632Push::emit, but looks like some
> > tests currently don't handle that yet?
> > 
> > ====
> > 
> > Assert from push::emit:
> > 
> > ${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
> > ${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/cmp-opt.ll -o -
|
> > llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -
> > 
> > <stdin>:27:7: error: invalid operand for instruction
> >         push    dl
> >                 ^~
> > <stdin>:37:7: error: invalid operand for instruction
> >         push    bl
> >                 ^~
> > 
> > =====
> > 
> > Assert from lowerCall:
> > 
> > ${FOO}/toolchain_build/src/subzero/llvm2ice -O2 --verbose none
> > ${FOO}/toolchain_build/src/subzero/tests_lit/llvm2ice_tests/bool-opt.ll -o -
|
> > llvm-mc -arch=x86 -x86-asm-syntax=intel -filetype=obj -o=MyObj.o  -
> > <stdin>:16:7: error: invalid operand for instruction
> >         push    al
> >                 ^~
> > 	call	use
> > 
> 
> Hmm, looks like those two tests are not valid PNaCl bitcode since their use()
> function is declared to take an i1 arg. :(

Ah right, I should have looked at what the test was doing =)

I'll send a separate fix for the tests.

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/321993002/diff/190001/crosstest/runtests.sh#n...
crosstest/runtests.sh:66: "${OUTDIR}"/mem_intrin_O${optlevel}
On 2014/06/16 23:05:16, stichnot wrote:
> On 2014/06/16 20:51:59, jvoung wrote:
> > Should I put "test_" prefix?
> > 
> > Keep it sorted?
> 
> I guess I have no real preference here (except to append with _Om1 / _O2). 
And
> there was no real consistency before. :)

Alright, I'll leave the filename alone for now, to keep review churn down, but
sort the tests after simple_loop.  Keep simple_loop first since that's the
simplest and if that doesn't work we're in trouble =)

https://codereview.chromium.org/321993002/diff/320001/crosstest/mem_intrin.h
File crosstest/mem_intrin.h (right):

https://codereview.chromium.org/321993002/diff/320001/crosstest/mem_intrin.h#...
crosstest/mem_intrin.h:6: #include <stdint.h> /* cstdint requires -std=c++0x or
higher */
On 2014/06/16 23:05:16, stichnot wrote:
> Since mem_intrin.h is now being #included inside namespace Subzero_ {}, I
don't
> think you should be #including standard headers, right?  Or does C++ somehow
pop
> out of namespaces for these includes?

Hmm good point. I don't know of any way to pop namespaces. These were here to
provide the typedefs for uint8_t and size_t, which shouldn't be affected by
namespaces. Still, there's more to the headers than that so I'll remove them.

https://codereview.chromium.org/321993002/diff/320001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/321993002/diff/320001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:38: , "nacl.atomic.cmpxchg." #NameSuffix                  
                    \
On 2014/06/16 23:05:16, stichnot wrote:
> I don't think the # is necessary here, is it?  (here and below)

Done.

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

https://codereview.chromium.org/321993002/diff/320001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2174: // Vanilla ICE load instructions should not
use the segment registers,
On 2014/06/16 23:05:16, stichnot wrote:
> load --> store

Done.

[Jim Stichnoth, 2014-06-17 17:53:49]

lgtm

[jvoung (off chromium), 2014-06-18 17:51:03]

Committed patchset #18 manually as r3bd9f1a (presubmit successful).