Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1746063003:
[wasm] Add support for 64-bit LEB encodings. (Closed)
|
Created:
4 years, 9 months ago by titzer Modified:
4 years, 9 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Add support for 64-bit LEB encodings.
[email protected],[email protected]
BUG=
Committed: https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2
Cr-Commit-Position: refs/heads/master@{#34406}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 19
Patch Set 6 : #Messages
Total messages: 12 (3 generated)
PTAL
lgtm https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h File src/wasm/decoder.h (right): https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:111: return bit_cast<int64_t>(result << shift) >> shift; Could you add a comment to explain this line? Something like "// Use arithmetic shift to get sign extension." https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:335: const int kMaxDiff = (sizeof(IntType) * 8 + 6) / 7; Should this be "kMaxLength"? It is the maximum value of length, is it not? https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) - (sizeof(IntType) * 8); Could you add a comment here, to hint what this extra bits stuff is about? https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... File test/unittests/wasm/decoder-unittest.cc (right): https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:450: data[j] = (val >> (7 * j)) & MASK_7; static_cast? https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:486: TEST_F(DecoderTest, ReadI64v_TwoByte) { Should this be ReadI64v_OneByte? https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:569: TEST_F(DecoderTest, ReadI64v_Bits) { Would it be possible to write one test function for this test and the test "ReadU64v_Bits", maybe as a template function with a function pointer parameter?
https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h File src/wasm/decoder.h (right): https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:111: return bit_cast<int64_t>(result << shift) >> shift; On 2016/03/01 08:22:58, ahaas wrote: > Could you add a comment to explain this line? > Something like "// Use arithmetic shift to get sign extension." Done. https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:335: const int kMaxDiff = (sizeof(IntType) * 8 + 6) / 7; On 2016/03/01 08:22:58, ahaas wrote: > Should this be "kMaxLength"? It is the maximum value of length, is it not? Done. https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) - (sizeof(IntType) * 8); On 2016/03/01 08:22:58, ahaas wrote: > Could you add a comment here, to hint what this extra bits stuff is about? Done. https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... File test/unittests/wasm/decoder-unittest.cc (right): https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:450: data[j] = (val >> (7 * j)) & MASK_7; On 2016/03/01 08:22:58, ahaas wrote: > static_cast? Done. https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:486: TEST_F(DecoderTest, ReadI64v_TwoByte) { On 2016/03/01 08:22:58, ahaas wrote: > Should this be ReadI64v_OneByte? Done.
curious about non-minimal leb128, otherwise lgtm https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h File src/wasm/decoder.h (right): https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:337: const byte* end = ptr + kMaxDiff; I don't think this is right if we allow non-minimal LEB (which was suggested in the discussion) https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) - (sizeof(IntType) * 8); Yeah, it took me walking through it to understand the purpose of the +1, it'd be nice to have a comment. :) https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... File test/unittests/wasm/decoder-unittest.cc (right): https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:444: // foreach length 1...64 32 https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:446: const uint32_t val = kVals[v] & ((1ull << (i - 1)) - 1); Am I reading this wrong, or does this give the wrong bitmasks? Looks like it's off by one.
https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h File src/wasm/decoder.h (right): https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc... src/wasm/decoder.h:337: const byte* end = ptr + kMaxDiff; On 2016/03/01 18:49:40, binji wrote: > I don't think this is right if we allow non-minimal LEB (which was suggested in > the discussion) I don't think we should support arbitrary-length LEB where stuff is expected to be 32 or 64 bits, so making the cutoff 5 or 10 bytes respectively seems OK. https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... File test/unittests/wasm/decoder-unittest.cc (right): https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:444: // foreach length 1...64 On 2016/03/01 18:49:40, binji wrote: > 32 Done. https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:446: const uint32_t val = kVals[v] & ((1ull << (i - 1)) - 1); On 2016/03/01 18:49:40, binji wrote: > Am I reading this wrong, or does this give the wrong bitmasks? > Looks like it's off by one. Good catch. It would strip off one extra high bit. https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec... test/unittests/wasm/decoder-unittest.cc:569: TEST_F(DecoderTest, ReadI64v_Bits) { On 2016/03/01 08:22:58, ahaas wrote: > Would it be possible to write one test function for this test and the test > "ReadU64v_Bits", maybe as a template function with a function pointer parameter? Too much mechanism I think.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1746063003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1746063003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1746063003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add support for 64-bit LEB encodings. [email protected],[email protected] BUG= ========== to ========== [wasm] Add support for 64-bit LEB encodings. [email protected],[email protected] BUG= Committed: https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2 Cr-Commit-Position: refs/heads/master@{#34406} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2 Cr-Commit-Position: refs/heads/master@{#34406}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1749343002/ by [email protected]. The reason for reverting is: [Sheriff] Seems to break chromium win compile: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Win/builds/380.... |
