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

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

[titzer, 2013-08-26 13:06:45]

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (left):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:3932: instr->SetGVNFlag(kDependsOnMaps);
Can a map transition have side effects other than just changing that one word in
the header of that object, e.g. change the layout of an object, thus
invalidating a previously loaded value?

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:3907: // Negative property indices are in-object
properties, indexed
I think I prefer the if; the logic gets too twisted otherwise.

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:5592: class OffsetField : public BitField<int, 7,
26> {};
Not gonna fit in 32 anymore, unless you shrink the offset yet more :(

[Toon Verwaest, 2013-08-26 13:39:16]

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (left):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:3932: instr->SetGVNFlag(kDependsOnMaps);
No. We are transitioning because that property didn't exist yet for that object.
Hence it can't change a value of a property loaded from that object.

Maps do change however, so DeopendsOnMaps values can't be hoisted over it.
HCheckMaps eg does depend on maps. And HLoadNamedField depend on HCheckMaps.

For some reason kChangesMaps is still set manually in BuildStoreNamedField.

On 2013/08/26 13:06:45, titzer wrote:
> Can a map transition have side effects other than just changing that one word
in
> the header of that object, e.g. change the layout of an object, thus
> invalidating a previously loaded value?

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:3907: // Negative property indices are in-object
properties, indexed
Ok, I'll revert to it.

On 2013/08/26 13:06:45, titzer wrote:
> I think I prefer the if; the logic gets too twisted otherwise.

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/23241027/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:5592: class OffsetField : public BitField<int, 7,
26> {};
Woops. 25 should be more than enough, given that we can only have 2^11 fast-mode
properties.

On 2013/08/26 13:06:45, titzer wrote:
> Not gonna fit in 32 anymore, unless you shrink the offset yet more :(

[Toon Verwaest, 2013-08-26 15:17:36]

Addressed comments, PTAL again.

[titzer, 2013-08-26 15:33:47]

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.