Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571883002:
[stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). (Closed)
Description[stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol().
Committed: https://crrev.com/3b1a09f56d5c94d1ada067a5b6ea15487673e3c2
Cr-Commit-Position: refs/heads/master@{#41682}
Patch Set 1 #Patch Set 2 : refactor #Patch Set 3 : fix test #Patch Set 4 : remove stuff #
Total comments: 12
Patch Set 5 : refactor #
Messages
Total messages: 29 (22 generated)
The CQ bit was checked by gsathya@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...
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org, ishell@chromium.org
The CQ bit was checked by gsathya@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: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by gsathya@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 checked by gsathya@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...
Description was changed from ========== [csa] Add IsSymbol and IsPrivateSymbol ========== to ========== [stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I updated CL description. lgtm with nits: https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2919: return HasInstanceType(object, SYMBOL_TYPE); IsSymbolMap(LoadMap(object)) does not require loading an instance type field. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2924: Variable var_result(this, MachineType::PointerRepresentation()); CSA expects kWord32 nodes as a condition in Branch, so the result should be a kWord32. (The CSA graph verification is almost there). https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2925: var_result.Bind(IntPtrConstant(0)); Same here: Int32Constant(). https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2927: GotoUnless(IsSymbolMap(LoadMap(object)), &out); IsSymbol(object) https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2935: return var_result.value(); You may want to use Select() for this. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:4296: Node* key_instance_type = LoadInstanceType(key); On this path we are loading map twice. How about: LoadMap, IsSymbolMap, LoadMapInstanceType?
The CQ bit was checked by gsathya@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...
https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2919: return HasInstanceType(object, SYMBOL_TYPE); On 2016/12/13 20:56:28, Igor Sheludko wrote: > IsSymbolMap(LoadMap(object)) does not require loading an instance type field. Doh ofc. Changed https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2924: Variable var_result(this, MachineType::PointerRepresentation()); On 2016/12/13 20:56:28, Igor Sheludko wrote: > CSA expects kWord32 nodes as a condition in Branch, so the result should be a > kWord32. (The CSA graph verification is almost there). Ah, okay. Changed. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2925: var_result.Bind(IntPtrConstant(0)); On 2016/12/13 20:56:28, Igor Sheludko wrote: > Same here: Int32Constant(). Done. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2927: GotoUnless(IsSymbolMap(LoadMap(object)), &out); On 2016/12/13 20:56:28, Igor Sheludko wrote: > IsSymbol(object) Done. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:2935: return var_result.value(); On 2016/12/13 20:56:28, Igor Sheludko wrote: > You may want to use Select() for this. I did initially but didn't look very readable to me. Changed. https://codereview.chromium.org/2571883002/diff/50001/src/code-stub-assembler... src/code-stub-assembler.cc:4296: Node* key_instance_type = LoadInstanceType(key); On 2016/12/13 20:56:28, Igor Sheludko wrote: > On this path we are loading map twice. How about: LoadMap, IsSymbolMap, > LoadMapInstanceType? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1481670569077330,
"parent_rev": "a0b2199ef0f41e6b150fc9fd7f4611e7aaf2ea62", "commit_rev":
"887a344643fff9d80caf14dd8170a43ff0c61416"}
Message was sent while issue was closed.
Description was changed from ========== [stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). ========== to ========== [stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). Review-Url: https://codereview.chromium.org/2571883002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== [stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). Review-Url: https://codereview.chromium.org/2571883002 ========== to ========== [stubs] Add CSA::IsSymbol() and CSA::IsPrivateSymbol(). Committed: https://crrev.com/3b1a09f56d5c94d1ada067a5b6ea15487673e3c2 Cr-Commit-Position: refs/heads/master@{#41682} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3b1a09f56d5c94d1ada067a5b6ea15487673e3c2 Cr-Commit-Position: refs/heads/master@{#41682} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
