Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465103002:
[wasm] remove MachineType from asm-wasm-builder.cc (Closed)
Description[wasm] remove MachineType from asm-wasm-builder.cc
R=bradnelson@chromium.org
BUG=
Committed: https://crrev.com/8d661a339fdaf5a609e70196deb90345875d51ed
Cr-Commit-Position: refs/heads/master@{#40849}
Patch Set 1 #
Total comments: 6
Patch Set 2 : [wasm] remove MachineType from asm-wasm-builder.cc #
Total comments: 4
Patch Set 3 : [wasm] remove MachineType from asm-wasm-builder.cc #
Depends on Patchset: Messages
Total messages: 22 (11 generated)
The CQ bit was checked by aseemgarg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/01 01:11:06, aseemgarg wrote: Seems like this is increasing the runtime of some of the benchmarks too. But not a lot. Could just be noise.
Description was changed from ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= ========== to ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= ==========
bradnelson@chromium.org changed reviewers: + jpp@chromium.org
John, I wanted to nix the machine types completely. The cost of figuring out what wasm opcode has turned out to matter a tiny bit. Any opinion on the right way to do these? https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.cc File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:852: if (type->IsA(AsmType::Int8Array())) { IsA is more expensive (since it has a bunch of fallbacks) so this isn't surprising. For basic types these should have unique values. You should be able to do: type == AsmType::Int8Array() etc for these. Though maybe we should do something specific in asm-types.h. I'll CC jpp on this and see what he thinks. https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:967: if (type->IsA(AsmType::Uint8Array())) { Switch this to calling: size = type->ElementSizeInBytes(); IsA is not as cheap (it permits more doesn't turn into a switch). https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:1026: if (type->IsA(AsmType::Int8Array())) { Same as above, IsA isn't cheap.
https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.cc File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:852: if (type->IsA(AsmType::Int8Array())) { On 2016/11/02 13:24:22, bradnelson wrote: > IsA is more expensive (since it has a bunch of fallbacks) so this isn't > surprising. > For basic types these should have unique values. > You should be able to do: > type == AsmType::Int8Array() etc for these. > > Though maybe we should do something specific in asm-types.h. > I'll CC jpp on this and see what he thinks. Done. https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:967: if (type->IsA(AsmType::Uint8Array())) { On 2016/11/02 13:24:22, bradnelson wrote: > Switch this to calling: > > size = type->ElementSizeInBytes(); > > IsA is not as cheap (it permits more doesn't turn into a switch). Done. https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:1026: if (type->IsA(AsmType::Int8Array())) { On 2016/11/02 13:24:22, bradnelson wrote: > Same as above, IsA isn't cheap. Done.
Results all over the place after getting rid of isA too (that should be ok but for some benchmarks there is a regression of over 1%). Please see: v8_linux32_perf_try https://uberchromegw.corp.google.com/i/internal.client.v8/builders/v8_linux32... v8_linux64_perf_try https://uberchromegw.corp.google.com/i/internal.client.v8/builders/v8_linux64... On 2016/11/02 19:40:01, aseemgarg wrote: > https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.cc > File src/asmjs/asm-wasm-builder.cc (right): > > https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... > src/asmjs/asm-wasm-builder.cc:852: if (type->IsA(AsmType::Int8Array())) { > On 2016/11/02 13:24:22, bradnelson wrote: > > IsA is more expensive (since it has a bunch of fallbacks) so this isn't > > surprising. > > For basic types these should have unique values. > > You should be able to do: > > type == AsmType::Int8Array() etc for these. > > > > Though maybe we should do something specific in asm-types.h. > > I'll CC jpp on this and see what he thinks. > > Done. > > https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... > src/asmjs/asm-wasm-builder.cc:967: if (type->IsA(AsmType::Uint8Array())) { > On 2016/11/02 13:24:22, bradnelson wrote: > > Switch this to calling: > > > > size = type->ElementSizeInBytes(); > > > > IsA is not as cheap (it permits more doesn't turn into a switch). > > Done. > > https://codereview.chromium.org/2465103002/diff/1/src/asmjs/asm-wasm-builder.... > src/asmjs/asm-wasm-builder.cc:1026: if (type->IsA(AsmType::Int8Array())) { > On 2016/11/02 13:24:22, bradnelson wrote: > > Same as above, IsA isn't cheap. > > Done.
bradnelson@google.com changed reviewers: + bradnelson@google.com
lgtm https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... src/asmjs/asm-wasm-builder.cc:22: #include "src/codegen.h" This can be dropped.
lgtm https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... src/asmjs/asm-wasm-builder.cc:854: } else if (type == AsmType::Uint8Array()) { We could optimize the array type in asm-types as they don't need a separate bit for each size (which probably results in this needing to be more expensive than it currently is).
https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... src/asmjs/asm-wasm-builder.cc:22: #include "src/codegen.h" On 2016/11/08 17:55:45, bradn wrote: > This can be dropped. Done. https://codereview.chromium.org/2465103002/diff/20001/src/asmjs/asm-wasm-buil... src/asmjs/asm-wasm-builder.cc:854: } else if (type == AsmType::Uint8Array()) { On 2016/11/08 17:59:22, bradnelson wrote: > We could optimize the array type in asm-types as they don't need a separate bit > for each size (which probably results in this needing to be more expensive than > it currently is). I think you are addressing this as a separate patch?
The CQ bit was checked by aseemgarg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org, bradnelson@google.com Link to the patchset: https://codereview.chromium.org/2465103002/#ps40001 (title: "[wasm] remove MachineType from asm-wasm-builder.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= ========== to ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= ========== to ========== [wasm] remove MachineType from asm-wasm-builder.cc R=bradnelson@chromium.org BUG= Committed: https://crrev.com/8d661a339fdaf5a609e70196deb90345875d51ed Cr-Commit-Position: refs/heads/master@{#40849} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8d661a339fdaf5a609e70196deb90345875d51ed Cr-Commit-Position: refs/heads/master@{#40849} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
