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

Issue 23241027: Avoid setting / depending on flags when transitioning, and when fields are known to be constant. (Closed)

Created:
7 years, 3 months ago by Toon Verwaest
Modified:
7 years, 3 months ago
Reviewers:
titzer
CC:
v8-dev
Visibility:
Public.

Description

Avoid setting / depending on flags when transitioning, and when fields are known to be constant.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed comment #

Patch Set 3 : Fix size. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -18 lines) Patch
M src/hydrogen.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M src/hydrogen-instructions.h View 1 2 4 chunks +16 lines, -6 lines 0 comments Download
M src/hydrogen-instructions.cc View 1 3 chunks +16 lines, -9 lines 1 comment Download

Messages

Total messages: 4 (0 generated)
titzer
https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (left): https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructions.cc#oldcode3932 src/hydrogen-instructions.cc:3932: instr->SetGVNFlag(kDependsOnMaps); Can a map transition have side effects other ...
7 years, 3 months ago (2013-08-26 13:06:45 UTC) #1
Toon Verwaest
https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (left): https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructions.cc#oldcode3932 src/hydrogen-instructions.cc:3932: instr->SetGVNFlag(kDependsOnMaps); No. We are transitioning because that property didn't ...
7 years, 3 months ago (2013-08-26 13:39:16 UTC) #2
Toon Verwaest
Addressed comments, PTAL again.
7 years, 3 months ago (2013-08-26 15:17:36 UTC) #3
titzer
7 years, 3 months ago (2013-08-26 15:33:47 UTC) #4
https://chromiumcodereview.appspot.com/23241027/diff/7001/src/hydrogen-instru...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/23241027/diff/7001/src/hydrogen-instru...
src/hydrogen-instructions.cc:3940: if (FieldTypeField::decode(value_) ==
CONSTANT) return;
Mmmm, this is starting to make me feel uncomfortable.

Here is the problem. With a CONSTANT load, it no longer depends on _any_ GVN
flags. But you can also have a CONSTANT store, which doesn't change _any_ GVN
flags (but does depend on NewSpacePromotion--which is somewhat weird). The
problem is that once those flags are gone, all bets are off. The load could get
lifted above the store, and then it will get the uninitialized value, not the
stored value.

We are likely to get lucky just because GVN and LICM always lift instructions in
their original order. That is not always going to be the case.

We should express the load/store dependency here with an explicit edge--i.e. the
load depends directly on the store.

In the future we will have load/store elimination, but even then, we need to
make this dependency explicit.

Powered by Google App Engine
This is Rietveld 408576698