Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251833002:
[turbofan] Only do value numbering when types are compatible. (Closed)
Description[turbofan] Only do value numbering when types are compatible.
At the moment, two NumberConstant nodes get different type even if their
value is the same because we always allocate a new heap number for
each number constant. This can lead to replacing a node with a node of
disjoint type in value numbering, which can result in incorrect code
down the line because of inconsistent types.
This fix makes sure that we only replace a node with a sub-type
node. Once we introduce a proper type for number constants, we can
move back to the intersection typing in value numbering.
Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes).
BUG=chromium:633497
Committed: https://crrev.com/b190d13331ba454a416e03c63804f1873252b24f
Cr-Commit-Position: refs/heads/master@{#38675}
Patch Set 1 #Patch Set 2 : Review comments #
Total comments: 1
Messages
Total messages: 22 (15 generated)
The CQ bit was checked by jarin@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 ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. ========== to ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. BUG=chromium:633497 ==========
jarin@chromium.org changed reviewers: + neis@chromium.org
Could you take a look, please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. BUG=chromium:633497 ========== to ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bites of their hashes). BUG=chromium:633497 ==========
The CQ bit was checked by jarin@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...
lgtm. Awesome test!
On 2016/08/16 21:17:06, Jarin wrote: > Could you take a look, please? In an offline discussion, @neis suggested we can still narrow to the smaller type if the types are comparable. Patch set 2 does that.
Description was changed from ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bites of their hashes). BUG=chromium:633497 ========== to ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes). BUG=chromium:633497 ==========
The CQ bit was unchecked by jarin@chromium.org
The CQ bit was checked by jarin@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.
Description was changed from ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes). BUG=chromium:633497 ========== to ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes). BUG=chromium:633497 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes). BUG=chromium:633497 ========== to ========== [turbofan] Only do value numbering when types are compatible. At the moment, two NumberConstant nodes get different type even if their value is the same because we always allocate a new heap number for each number constant. This can lead to replacing a node with a node of disjoint type in value numbering, which can result in incorrect code down the line because of inconsistent types. This fix makes sure that we only replace a node with a sub-type node. Once we introduce a proper type for number constants, we can move back to the intersection typing in value numbering. Unfortunately, it is quite hard to write a repro for this because we cache NumberConstant nodes. We only throw away cached values that have too many conflicts (>5), so the test has to contain values that fall into the same bucket. That's where the magic floating point numbers in the test come from (they have the same low 8-bits of their hashes). BUG=chromium:633497 Committed: https://crrev.com/b190d13331ba454a416e03c63804f1873252b24f Cr-Commit-Position: refs/heads/master@{#38675} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b190d13331ba454a416e03c63804f1873252b24f Cr-Commit-Position: refs/heads/master@{#38675}
Message was sent while issue was closed.
ahaas@chromium.org changed reviewers: + ahaas@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2251833002/diff/20001/test/mjsunit/compiler/r... File test/mjsunit/compiler/regress-633497.js (right): https://codereview.chromium.org/2251833002/diff/20001/test/mjsunit/compiler/r... test/mjsunit/compiler/regress-633497.js:16: dummy = 527.5; Could we add a comment which describes the purpose of these dummy values? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
