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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 684193002: [turbofan] Fix input count in Uint32Mod/Div reduction. (Closed)

Created:
6 years, 1 month ago by Jarin
Modified:
6 years, 1 month ago
Reviewers:
Benedikt Meurer
CC:
v8-dev, titzer
Project:
v8
Visibility:
Public.

Description

[turbofan] Fix input count in Uint32Mod/Div reduction. BUG= R=bmeurer@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=24997

Patch Set 1 #

Patch Set 2 : Fix Uint32Div, too #

Total comments: 4

Patch Set 3 : Rebase #

Patch Set 4 : Change order of node transformation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -8 lines) Patch
M src/compiler/machine-operator-reducer.cc View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M test/cctest/compiler/test-machine-operator-reducer.cc View 1 2 10 chunks +21 lines, -8 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
Jarin
Benedikt, could you take a look, please?
6 years, 1 month ago (2014-10-29 15:20:19 UTC) #2
Benedikt Meurer
LGTM with comments. https://codereview.chromium.org/684193002/diff/20001/src/compiler/machine-operator-reducer.cc File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/684193002/diff/20001/src/compiler/machine-operator-reducer.cc#newcode603 src/compiler/machine-operator-reducer.cc:603: node->ReplaceInput(1, Uint32Constant(WhichPowerOf2(m.right().Value()))); Let's keep the set_op() ...
6 years, 1 month ago (2014-10-29 18:26:45 UTC) #3
Jarin
Committed patchset #4 (id:60001) manually as 24997 (presubmit successful).
6 years, 1 month ago (2014-10-29 21:07:26 UTC) #4
Jarin
6 years, 1 month ago (2014-10-29 21:12:24 UTC) #5
Message was sent while issue was closed. 
https://codereview.chromium.org/684193002/diff/20001/src/compiler/machine-ope...
File src/compiler/machine-operator-reducer.cc (right):

https://codereview.chromium.org/684193002/diff/20001/src/compiler/machine-ope...
src/compiler/machine-operator-reducer.cc:603: node->ReplaceInput(1,
Uint32Constant(WhichPowerOf2(m.right().Value())));
On 2014/10/29 18:26:45, Benedikt Meurer wrote:
> Let's keep the set_op() before ReplaceInput.

Done.

I am wondering what's the reason for this. I was always imagining that the
set_op method should be the operation that morally 'commits' the changes to the
node. E.g., if we want to hook up the typer for re-typing, set_op would be the
method to do that. (Well, we would have to make set_op a method on the graph,
but you get the idea.)

https://codereview.chromium.org/684193002/diff/20001/src/compiler/machine-ope...
src/compiler/machine-operator-reducer.cc:671:
node->set_op(machine()->Word32And());
On 2014/10/29 18:26:45, Benedikt Meurer wrote:
> Same here.

Done.

Powered by Google App Engine
This is Rietveld 408576698