Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(173)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 576853002: Add forward instruction declaration to Subzero bitcode reader. (Closed)

Created:
6 years, 3 months ago by Karl
Modified:
6 years, 3 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Fix nits and clean up tests. #

Patch Set 3 : Format differences. #

Total comments: 6

Patch Set 4 : Fix nits in patch set 3. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -13 lines) Patch
M src/PNaClTranslator.cpp View 1 7 chunks +94 lines, -12 lines 0 comments Download
M tests_lit/lit.cfg View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
A tests_lit/reader_tests/forwardref.ll View 1 2 3 1 chunk +122 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
Karl
6 years, 3 months ago (2014-09-16 21:08:22 UTC) #2
Jim Stichnoth
lgtm https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/forwardref.ll File tests_lit/reader_tests/forwardref.ll (right): https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/forwardref.ll#newcode34 tests_lit/reader_tests/forwardref.ll:34: ; DUMP: %b0: DUMP-NEXT? https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/forwardref.ll#newcode90 tests_lit/reader_tests/forwardref.ll:90: ; DUMP: ...
6 years, 3 months ago (2014-09-16 21:30:24 UTC) #3
jvoung (off chromium)
LGTM https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/forwardref.ll File tests_lit/reader_tests/forwardref.ll (right): https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/forwardref.ll#newcode9 tests_lit/reader_tests/forwardref.ll:9: ; RUN: llvm-as < %s | pnacl-freeze | ...
6 years, 3 months ago (2014-09-16 21:43:41 UTC) #4
Karl
Committed patchset #4 (id:60001) manually as 8f07aa8 (presubmit successful).
6 years, 3 months ago (2014-09-17 16:07:27 UTC) #5
Karl
6 years, 3 months ago (2014-09-17 16:07:38 UTC) #6
Message was sent while issue was closed. 
https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/f...
File tests_lit/reader_tests/forwardref.ll (right):

https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/f...
tests_lit/reader_tests/forwardref.ll:9: ; RUN: llvm-as < %s | pnacl-freeze |
pnacl-bcdis -no-records \
On 2014/09/16 21:43:41, jvoung wrote:
> Do we need to add pnacl-bcdis to tests_lit/lit.cfg anymore?

Added.

https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/f...
tests_lit/reader_tests/forwardref.ll:34: ; DUMP:        %b0:
On 2014/09/16 21:30:24, stichnot wrote:
> DUMP-NEXT?

I intensionally used DUMP because there are a number of records that appear
before instructions (i.e. a constants block). Adding elided text back.

https://codereview.chromium.org/576853002/diff/40001/tests_lit/reader_tests/f...
tests_lit/reader_tests/forwardref.ll:90: ; DUMP:        %b0:
On 2014/09/16 21:30:24, stichnot wrote:
> DUMP-NEXT?

Same here.

Powered by Google App Engine
This is Rietveld 408576698