Add alloca instruction to Subzero bitcode reader.

Add alloca instruction to Subzero bitcode reader.

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

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

[Karl, 2014-09-05 16:06:00]

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

[jvoung (off chromium), 2014-09-05 16:55:04]

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1515: if (!isValidRecordSize(2, "function block
alloca")) return;
return on separate line

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1524: unsigned Align = static_cast<unsigned>(Values[1]);
Should probably check that Align <= 29 or so? I think the max that LLVM supports
(for arbitrary reasons) is 2 ** 29:
http://llvm.org/docs/LangRef.html#alloca-instruction

You don't want to shift "1 << 33" below, for example.

https://codereview.chromium.org/545623005/diff/1/tests_lit/reader_tests/alloc.ll
File tests_lit/reader_tests/alloc.ll (right):

https://codereview.chromium.org/545623005/diff/1/tests_lit/reader_tests/alloc...
tests_lit/reader_tests/alloc.ll:57: %array = alloca i8, i32 %n, align 16
Once you have the constants block would be good to have a test for the constant
case, since that's the more typical use of alloca.

[Jim Stichnoth, 2014-09-05 23:03:39]

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1526: Inst = Ice::InstAlloca::create(Func, ByteCount, (1
<< Align) >> 1, Dest);
Any reason why it isn't (1<<(Align-1)) as opposed to ((1<<Align)>>1)?

What happens when Align==0?  (This would be the major difference between the two
expression, I think.)

The LLVM alloca documentation doesn't say that the alignment must be a power of
2.  Is that a PNaCl simplification, or is the LLVM documentation wrong?

[Karl, 2014-09-08 21:16:23]

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1515: if (!isValidRecordSize(2, "function block
alloca")) return;
On 2014/09/05 16:55:04, jvoung wrote:
> return on separate line

Done.

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1524: unsigned Align = static_cast<unsigned>(Values[1]);
On 2014/09/05 16:55:04, jvoung wrote:
> Should probably check that Align <= 29 or so? I think the max that LLVM
supports
> (for arbitrary reasons) is 2 ** 29:
> http://llvm.org/docs/LangRef.html#alloca-instruction
> 
> You don't want to shift "1 << 33" below, for example.

Good point.

https://codereview.chromium.org/545623005/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1526: Inst = Ice::InstAlloca::create(Func, ByteCount, (1
<< Align) >> 1, Dest);
On 2014/09/05 23:03:39, stichnot wrote:
> Any reason why it isn't (1<<(Align-1)) as opposed to ((1<<Align)>>1)?
> 
> What happens when Align==0?  (This would be the major difference between the
two
> expression, I think.)
> 
> The LLVM alloca documentation doesn't say that the alignment must be a power
of
> 2.  Is that a PNaCl simplification, or is the LLVM documentation wrong?

In LLVM and PNaCl, 0 is allowed, and hence the special calculation.
Also, while LLVM isn't explicit about the power of 2 for alloca, it is
elsewhere. In addition, the LLVM bitcode writer explicitly assumes this and
converts it to the nearest enclosing power of 2. I will make sure that we make
this explicit in the PNaCl documentation.

https://codereview.chromium.org/545623005/diff/1/tests_lit/reader_tests/alloc.ll
File tests_lit/reader_tests/alloc.ll (right):

https://codereview.chromium.org/545623005/diff/1/tests_lit/reader_tests/alloc...
tests_lit/reader_tests/alloc.ll:7: 
Added case of alignment == 0, since it is allowed.

https://codereview.chromium.org/545623005/diff/1/tests_lit/reader_tests/alloc...
tests_lit/reader_tests/alloc.ll:57: %array = alloca i8, i32 %n, align 16
On 2014/09/05 16:55:04, jvoung wrote:
> Once you have the constants block would be good to have a test for the
constant
> case, since that's the more typical use of alloca.

Done.

[jvoung (off chromium), 2014-09-08 23:31:23]

Otherwise LGTM

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1535: Alignment = (1 <<
static_cast<unsigned>(AlignPower)) >> 1;
Isn't AlignPower already "unsigned" (cast was originally around Values[1])

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
File tests_lit/reader_tests/alloca.ll (right):

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
tests_lit/reader_tests/alloca.ll:15: ; CHECK:      __0:
Hmm, most of the tests do "CHECK-LABEL: function_name", instead of CHECK: __0:,
so this seems a little unusual. Why not use the function_name instead for
consistency? Because of the CHECK-NEXT setup?

[Jim Stichnoth, 2014-09-08 23:54:41]

otherwise lgtm

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1535: Alignment = (1 <<
static_cast<unsigned>(AlignPower)) >> 1;
static_cast<> is unnecessary.

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
File tests_lit/reader_tests/alloca.ll (right):

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
tests_lit/reader_tests/alloca.ll:15: ; CHECK:      __0:
On 2014/09/08 23:31:23, jvoung wrote:
> Hmm, most of the tests do "CHECK-LABEL: function_name", instead of CHECK:
__0:,
> so this seems a little unusual. Why not use the function_name instead for
> consistency? Because of the CHECK-NEXT setup?

I think CHECK-LABEL is only appropriate for emitted code, but this is testing
just the bitcode dump.

Along those lines, it's better to put a named "entry:" label in the entry basic
block if you eventually want to use szdiff.py, but I suspect that labels aren't
yet supported.

[Karl, 2014-09-09 18:40:17]

Committed patchset #3 (id:40001) manually as 742d72d (presubmit successful).

[Karl, 2014-09-10 17:29:59]

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1535: Alignment = (1 <<
static_cast<unsigned>(AlignPower)) >> 1;
On 2014/09/08 23:31:23, jvoung wrote:
> Isn't AlignPower already "unsigned" (cast was originally around Values[1])

Actually, I meant to make AlignPower uint64_t. Otherwise, it could contain a
VERY large number that would get lost in the (implicit) cast.
Changing tyep of AlignPower to uint64_t.

https://codereview.chromium.org/545623005/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1535: Alignment = (1 <<
static_cast<unsigned>(AlignPower)) >> 1;
On 2014/09/08 23:54:40, stichnot wrote:
> static_cast<> is unnecessary.

See other comment for line.

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
File tests_lit/reader_tests/alloca.ll (right):

https://codereview.chromium.org/545623005/diff/20001/tests_lit/reader_tests/a...
tests_lit/reader_tests/alloca.ll:15: ; CHECK:      __0:
On 2014/09/08 23:54:40, stichnot wrote:
> On 2014/09/08 23:31:23, jvoung wrote:
> > Hmm, most of the tests do "CHECK-LABEL: function_name", instead of CHECK:
> __0:,
> > so this seems a little unusual. Why not use the function_name instead for
> > consistency? Because of the CHECK-NEXT setup?
> 
> I think CHECK-LABEL is only appropriate for emitted code, but this is testing
> just the bitcode dump.
> 
> Along those lines, it's better to put a named "entry:" label in the entry
basic
> block if you eventually want to use szdiff.py, but I suspect that labels
aren't
> yet supported.

Since we aren't handling valuesymtab blocks within functions yet, we can't give
them a good name.