Add constants block to PNaCl bitcode reader.

Add constants block to PNaCl 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=f12355e

[Karl, 2014-09-05 16:41:35]

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

[Jim Stichnoth, 2014-09-05 23:34:29]

lgtm

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

https://codereview.chromium.org/548553002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1557: if (NextConstantType != Ice::IceType_void) return
true;
return on separate line, i.e. run "make format-diff"

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

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:8: 
A lot of blank lines here...

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:15: define void @TestIntegers() {
I should point out that Subzero is not checking that the constant value fits
within the bounds of its corresponding type.  It sort of gets checked by the
assembler after code emission.  There should probably be checks in
GlobalContext::getConstant{Int,Float,Double}().

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:75: ; CHECK-NEXT:   %__0 = fadd float
1.000000e+00, 0.000000e+00
Subzero's FP dump routines don't try to be precise like LLVM's, but it's
probably OK for a smoke test.

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:118: ; CHECK-NEXT:   %__11 = fadd double
-inf, 0.000000e+00
Other interesting FP constants include negative NaN, negative zero, FLT_MIN,
FLT_MAX, DBL_MIN, DBL_MAX.

[jvoung (off chromium), 2014-09-06 17:53:47]

LGTM too

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

https://codereview.chromium.org/548553002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1588: Context->convertToLLVMType(NextConstantType))) {
Might just be able to check the IceType's typeWidthInBytes() * CHAR_BIT (with
the exception of i1), to reduce one use of dependence on LLVM types. Would still
need to check it's an integer type though.

Or we can wait and stick with this for now.

[Karl, 2014-09-08 20:41:17]

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

[Karl, 2014-09-08 20:41:39]

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

https://codereview.chromium.org/548553002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1557: if (NextConstantType != Ice::IceType_void) return
true;
On 2014/09/05 23:34:29, stichnot wrote:
> return on separate line, i.e. run "make format-diff"

Done.

https://codereview.chromium.org/548553002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1588: Context->convertToLLVMType(NextConstantType))) {
On 2014/09/06 17:53:47, jvoung wrote:
> Might just be able to check the IceType's typeWidthInBytes() * CHAR_BIT (with
> the exception of i1), to reduce one use of dependence on LLVM types. Would
still
> need to check it's an integer type though.
> 
> Or we can wait and stick with this for now.

Since we already depend of LLVM typesfor the types block and function address
signatures, I will leave it in for the moment.

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

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:8: 
On 2014/09/05 23:34:29, stichnot wrote:
> A lot of blank lines here...

Removed.

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:15: define void @TestIntegers() {
On 2014/09/05 23:34:29, stichnot wrote:
> I should point out that Subzero is not checking that the constant value fits
> within the bounds of its corresponding type.  It sort of gets checked by the
> assembler after code emission.  There should probably be checks in
> GlobalContext::getConstant{Int,Float,Double}().

Note that we are already using APInt and APFloat (with the correct number of
bits argument) when we pars constants, we shouldn't be passing in illegal values
to GlobalContext::getConstant in the constants parser. So, for now, I don't see
a problem.

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:75: ; CHECK-NEXT:   %__0 = fadd float
1.000000e+00, 0.000000e+00
On 2014/09/05 23:34:29, stichnot wrote:
> Subzero's FP dump routines don't try to be precise like LLVM's, but it's
> probably OK for a smoke test.

Agreed. Not changing since this test is for parsing constant blocks, not
printing values.

https://codereview.chromium.org/548553002/diff/20001/tests_lit/reader_tests/c...
tests_lit/reader_tests/constants.ll:118: ; CHECK-NEXT:   %__11 = fadd double
-inf, 0.000000e+00
On 2014/09/05 23:34:29, stichnot wrote:
> Other interesting FP constants include negative NaN, negative zero, FLT_MIN,
> FLT_MAX, DBL_MIN, DBL_MAX.

Done.