Add call instructions to Subzero's bitcode reader.

Add call instructions to Subzero's bitcode reader.

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

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

[Karl, 2014-09-18 20:30:10]

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

[Jim Stichnoth, 2014-09-18 21:41:54]

LGTM, but be sure Jan is OK with the intrinsic changes.

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode299
src/IceInst.h:299: Operand *CallTarget, bool hasTailCall = false) {
Can you make this a non-default argument?  I think there's only one other place
that would need to be changed.

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode328
src/IceInst.h:328: bool hasTailCall;
Capitalize per LLVM convention.

Also, small nit - isn't it more proper to say it *is* a tail call, rather than
it *has* a tail call?

So may I suggest:
  IsTailCall for the field name
  TailCall for the create()/ctor arg

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
File tests_lit/reader_tests/call.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/call.ll:9: define i32 @fib(i32 %n) {
Just so I don't have to pore through all these tests...  Are there any
differences between these and the ones with the same name in the lit tests? 
(other than the RUN and CHECK lines)

[Karl, 2014-09-18 22:01:51]

I will wait for Jan's feedback before landing.

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode299
src/IceInst.h:299: Operand *CallTarget, bool hasTailCall = false) {
On 2014/09/18 21:41:54, stichnot wrote:
> Can you make this a non-default argument?  I think there's only one other
place
> that would need to be changed.

Done.

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode328
src/IceInst.h:328: bool hasTailCall;
On 2014/09/18 21:41:54, stichnot wrote:
> Capitalize per LLVM convention.
> 
> Also, small nit - isn't it more proper to say it *is* a tail call, rather than
> it *has* a tail call?
> 
> So may I suggest:
>   IsTailCall for the field name
>   TailCall for the create()/ctor arg

Done.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
File tests_lit/reader_tests/call.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/call.ll:9: define i32 @fib(i32 %n) {
On 2014/09/18 21:41:54, stichnot wrote:
> Just so I don't have to pore through all these tests...  Are there any
> differences between these and the ones with the same name in the lit tests? 
> (other than the RUN and CHECK lines)

Almost. I removed duplicate calls to an intrinsic that was testing downstream
additional compilation issues (ones with suffices more_addressing, dead, align,
const_len, mergeable_load, const, ignored, multiple, with_arith, alloca, loop,
store).

[jvoung (off chromium), 2014-09-18 22:04:46]

Sorry, still looking through... a small nit for now =)

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode319
src/IceInst.h:319: bool HasTailCl, bool HasSideEff, InstKind Kind)
nit: I think we might as well just make the argument name "HasTailCall" and
"HasSideEffects"?

[jvoung (off chromium), 2014-09-18 23:43:54]

Otherwise LGTM (yay the last instruction type?)

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
File tests_lit/reader_tests/call.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/call.ll:9: define i32 @fib(i32 %n) {
On 2014/09/18 22:01:50, Karl wrote:
> On 2014/09/18 21:41:54, stichnot wrote:
> > Just so I don't have to pore through all these tests...  Are there any
> > differences between these and the ones with the same name in the lit tests? 
> > (other than the RUN and CHECK lines)
> 
> Almost. I removed duplicate calls to an intrinsic that was testing downstream
> additional compilation issues (ones with suffices more_addressing, dead,
align,
> const_len, mergeable_load, const, ignored, multiple, with_arith, alloca, loop,
> store).

Hmm I'm not sure you need to have calls to all the intrinsics, but okay.

Eventually we will switch the default reader over to this one, and then the
other tests will cover the same.

I think what would have been good to add is a test for a non-existent intrinsic,
or one with bad type signature.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
File tests_lit/reader_tests/nacl-atomic-intrinsics.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:12: ;
https://code.google.com/p/nativeclient/issues/detail?id=3929
You can remove this comment since you're not checking the x86-32 output (which
this comment about the lock prefix applies to).

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:38: ; The PNaCl IR requires all
atomic accesses to be naturally aligned.
You probably don't need this comment about x86.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:663: ; CHECK-NEXT: entry:
If you want, you only need the test_atomic_is_lock_free() and could skip the
test_not_lock_free().

https://codereview.chromium.org/577353003/diff/40001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/577353003/diff/40001/src/IceConverter.cpp#new...
src/IceConverter.cpp:578: switch (I->validateCall(Call)) {
It would be okay to have the index outparam version of validateCall() here? Then
you could report something more precise for WrongCallArgType and remove the
extra wrapper version which ignores the outparam.

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1876: // CALL has an explicit funtion address, while the
CALL_INDIRECT
funtion -> function

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1878: // looking up the type signature associated with
the funtion
funtion -> function

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1934: StrBuf << "Invalid PNaCL intrinsic call to " <<
Name;
PNaCL -> PNaCl

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1950: Ice::InstIntrinsicCall::create(Func, Values.size()
- ParamsStartIndex,
"Values.size() - ParamsStartIndex" is the same as "NumParams"?

[Karl, 2014-09-19 16:33:35]

Committed patchset #4 (id:60001) manually as 8df26f3 (tree was closed).

[Karl, 2014-09-19 20:29:52]

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/577353003/diff/20001/src/IceInst.h#newcode319
src/IceInst.h:319: bool HasTailCl, bool HasSideEff, InstKind Kind)
On 2014/09/18 22:04:46, jvoung wrote:
> nit: I think we might as well just make the argument name "HasTailCall" and
> "HasSideEffects"?

I agree with using HasTailCall. Changing field back.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
File tests_lit/reader_tests/call.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/call.ll:9: define i32 @fib(i32 %n) {
On 2014/09/18 23:43:53, jvoung wrote:
> On 2014/09/18 22:01:50, Karl wrote:
> > On 2014/09/18 21:41:54, stichnot wrote:
> > > Just so I don't have to pore through all these tests...  Are there any
> > > differences between these and the ones with the same name in the lit
tests? 
> > > (other than the RUN and CHECK lines)
> > 
> > Almost. I removed duplicate calls to an intrinsic that was testing
downstream
> > additional compilation issues (ones with suffices more_addressing, dead,
> align,
> > const_len, mergeable_load, const, ignored, multiple, with_arith, alloca,
loop,
> > store).
> 
> Hmm I'm not sure you need to have calls to all the intrinsics, but okay.
> 
> Eventually we will switch the default reader over to this one, and then the
> other tests will cover the same.
> 
> I think what would have been good to add is a test for a non-existent
intrinsic,
> or one with bad type signature.

I can't test a bad type signature, because llvm-as complains about this.
However, added file nacl-fake-intrinsic.ll that tests for a non-existent
intrinsic.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
File tests_lit/reader_tests/nacl-atomic-intrinsics.ll (right):

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:12: ;
https://code.google.com/p/nativeclient/issues/detail?id=3929
On 2014/09/18 23:43:53, jvoung wrote:
> You can remove this comment since you're not checking the x86-32 output (which
> this comment about the lock prefix applies to).

Done.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:38: ; The PNaCl IR requires all
atomic accesses to be naturally aligned.
On 2014/09/18 23:43:53, jvoung wrote:
> You probably don't need this comment about x86.

Done.

https://codereview.chromium.org/577353003/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-atomic-intrinsics.ll:663: ; CHECK-NEXT: entry:
On 2014/09/18 23:43:53, jvoung wrote:
> If you want, you only need the test_atomic_is_lock_free() and could skip the
> test_not_lock_free().

Removed.

https://codereview.chromium.org/577353003/diff/40001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/577353003/diff/40001/src/IceConverter.cpp#new...
src/IceConverter.cpp:578: switch (I->validateCall(Call)) {
On 2014/09/18 23:43:53, jvoung wrote:
> It would be okay to have the index outparam version of validateCall() here?
Then
> you could report something more precise for WrongCallArgType and remove the
> extra wrapper version which ignores the outparam.

Done.

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1876: // CALL has an explicit funtion address, while the
CALL_INDIRECT
On 2014/09/18 23:43:53, jvoung wrote:
> funtion -> function

Done.

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1878: // looking up the type signature associated with
the funtion
On 2014/09/18 23:43:53, jvoung wrote:
> funtion -> function

Done.

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1934: StrBuf << "Invalid PNaCL intrinsic call to " <<
Name;
On 2014/09/18 23:43:53, jvoung wrote:
> PNaCL -> PNaCl

Done.

https://codereview.chromium.org/577353003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1950: Ice::InstIntrinsicCall::create(Func, Values.size()
- ParamsStartIndex,
On 2014/09/18 23:43:53, jvoung wrote:
> "Values.size() - ParamsStartIndex" is the same as "NumParams"?

Done.