Use shorter write-barrier filtering sequence when value is known to be non-smi.

Use shorter write-barrier filtering sequence when value is known to be non-smi.

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=20373

[Vyacheslav Egorov (Google), 2013-03-21 12:29:50]

ping.

[Ivan Posva, 2013-03-22 04:11:35]

LGTM with comments about naming.

-Ivan

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler.h
File runtime/vm/assembler.h (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler.h#newcod...
runtime/vm/assembler.h:200: enum ValueSminess {
You need to get a better name for this enum or just use a boolean.
Why do I think it is OK to use a boolean instead in this case: The value is not
set as a constant anywhere, it is always set using the result of a function
which has a Yes/No meaning.

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler_ia32.cc#...
runtime/vm/assembler_ia32.cc:1864: // ~value | object instead and skip the write
barrier if the bit is set.
Neat!

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler_ia32.cc#...
runtime/vm/assembler_ia32.cc:1904: StoreIntoObjectFilter(object, value, &done);
Please assert for kValueCanBeSmi in case we add more values to the enum. Or just
make the parameter a boolean.

https://codereview.chromium.org/12438032/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:2840: ValueSminess CanValueBeSmi() const {
Somehow I have an issue with the ValueSminess enum. Here your return a enum
result for something that clearly only has a boolean answer. "Can the value be
Smi?"

[Vyacheslav Egorov (Google), 2013-03-22 11:22:49]

Thanks. Addressed comments. Landing.

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler.h
File runtime/vm/assembler.h (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler.h#newcod...
runtime/vm/assembler.h:200: enum ValueSminess {
On 2013/03/22 04:11:35, Ivan Posva wrote:
> You need to get a better name for this enum or just use a boolean.
> Why do I think it is OK to use a boolean instead in this case: The value is
not
> set as a constant anywhere, it is always set using the result of a function
> which has a Yes/No meaning.

Killed enum, using boolean instead.

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler_ia32.cc
File runtime/vm/assembler_ia32.cc (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/assembler_ia32.cc#...
runtime/vm/assembler_ia32.cc:1904: StoreIntoObjectFilter(object, value, &done);
On 2013/03/22 04:11:35, Ivan Posva wrote:
> Please assert for kValueCanBeSmi in case we add more values to the enum. Or
just
> make the parameter a boolean.

Done.

https://codereview.chromium.org/12438032/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/12438032/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:2840: ValueSminess CanValueBeSmi() const {
On 2013/03/22 04:11:35, Ivan Posva wrote:
> Somehow I have an issue with the ValueSminess enum. Here your return a enum
> result for something that clearly only has a boolean answer. "Can the value be
> Smi?"

Done.

[Vyacheslav Egorov (Google), 2013-03-22 11:28:28]

Committed patchset #2 manually as r20373 (presubmit successful).