Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413343002:
[wasm] Use branch hint for the -1 check in I(32|64)Div. (Closed)
Description[wasm] Use branch hint for the -1 check in I(32|64)Div.
R=titzer@chromium.org
Committed: https://crrev.com/3ebb74e0a26e144239973a2c4fe3e8f5edb0e482
Cr-Commit-Position: refs/heads/master@{#40400}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use helper functions for branch hints #Patch Set 3 : Use anonymous namespace. #
Messages
Total messages: 23 (12 generated)
The CQ bit was checked by ahaas@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/10/13 at 18:31:18, titzer wrote: > I can only see an empty comment here. Did you actually add a comment?
https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1799: Node* is_denom_m1 = Can you add a branch hint to the old method so that we don't need to break it down to individual nodes here?
On 2016/10/17 at 13:44:16, titzer wrote: > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.cc > File src/compiler/wasm-compiler.cc (right): > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.... > src/compiler/wasm-compiler.cc:1799: Node* is_denom_m1 = > Can you add a branch hint to the old method so that we don't need to break it down to individual nodes here? I tried to. The problem was that since the declaration of {Branch} is in wasm-compiler.h, I cannot set a default value for a branch hint parameter. I also had a problem with the forward declaration of BranchHint, so I thought it's better to just inline Branch in these two locations.
On 2016/10/17 13:52:51, ahaas wrote: > On 2016/10/17 at 13:44:16, titzer wrote: > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.cc > > File src/compiler/wasm-compiler.cc (right): > > > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.... > > src/compiler/wasm-compiler.cc:1799: Node* is_denom_m1 = > > Can you add a branch hint to the old method so that we don't need to break it > down to individual nodes here? > > I tried to. The problem was that since the declaration of {Branch} is in > wasm-compiler.h, I cannot set a default value for a branch hint parameter. I > also had a problem with the forward declaration of BranchHint, so I thought it's > better to just inline Branch in these two locations. Can you at least make a file-local helper function? Manual construction of all the branch nodes is too verbose.
The CQ bit was checked by ahaas@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...
On 2016/10/17 at 13:54:17, titzer wrote: > On 2016/10/17 13:52:51, ahaas wrote: > > On 2016/10/17 at 13:44:16, titzer wrote: > > > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.cc > > > File src/compiler/wasm-compiler.cc (right): > > > > > > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.... > > > src/compiler/wasm-compiler.cc:1799: Node* is_denom_m1 = > > > Can you add a branch hint to the old method so that we don't need to break it > > down to individual nodes here? > > > > I tried to. The problem was that since the declaration of {Branch} is in > > wasm-compiler.h, I cannot set a default value for a branch hint parameter. I > > also had a problem with the forward declaration of BranchHint, so I thought it's > > better to just inline Branch in these two locations. > > Can you at least make a file-local helper function? Manual construction of all the branch nodes is too verbose. Done.
The CQ bit was checked by ahaas@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...
On 2016/10/18 12:09:29, ahaas wrote: > On 2016/10/17 at 13:54:17, titzer wrote: > > On 2016/10/17 13:52:51, ahaas wrote: > > > On 2016/10/17 at 13:44:16, titzer wrote: > > > > > > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.cc > > > > File src/compiler/wasm-compiler.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/2413343002/diff/1/src/compiler/wasm-compiler.... > > > > src/compiler/wasm-compiler.cc:1799: Node* is_denom_m1 = > > > > Can you add a branch hint to the old method so that we don't need to break > it > > > down to individual nodes here? > > > > > > I tried to. The problem was that since the declaration of {Branch} is in > > > wasm-compiler.h, I cannot set a default value for a branch hint parameter. I > > > also had a problem with the forward declaration of BranchHint, so I thought > it's > > > better to just inline Branch in these two locations. > > > > Can you at least make a file-local helper function? Manual construction of all > the branch nodes is too verbose. > > Done. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahaas@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Use branch hint for the -1 check in I(32|64)Div. R=titzer@chromium.org ========== to ========== [wasm] Use branch hint for the -1 check in I(32|64)Div. R=titzer@chromium.org Committed: https://crrev.com/3ebb74e0a26e144239973a2c4fe3e8f5edb0e482 Cr-Commit-Position: refs/heads/master@{#40400} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3ebb74e0a26e144239973a2c4fe3e8f5edb0e482 Cr-Commit-Position: refs/heads/master@{#40400} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
