Replace guarded list length field loads with constants

Replace guarded list length field loads with constants

R=fschneider@google.com, srdjan@google.com

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

[Florian Schneider, 2013-08-27 12:51:34]

https://codereview.chromium.org/23584002/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/23584002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:783: LoadFieldInstr* length =
If the field has a guarded_list_length: could you maybe just replace the length
LoadFieldInstr with a ConstantInstr? I think this would simplify the code a bit.

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

https://codereview.chromium.org/23584002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:6513: if (field_ == NULL) return
Field::kNoFixedLength;
Style suggestion:

return (field_ == NULL) ? Field::kNoFixedLength : field_->guarded_list_length();

[srdjan, 2013-08-27 15:50:33]

DBC

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

https://codereview.chromium.org/23584002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.cc:2523: if (max.UpperBound().value() <
known_array_length) {
How about "max.UpperBound().value() < GetGuardedArrayLength()"?

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

https://codereview.chromium.org/23584002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:6492: intptr_t deopt_id) : field_(field) {
Why store field instead of guarded_list _length or kNoFixedLength?

[Cutch, 2013-08-28 11:10:49]

PTAL at P.S.3 which relies on LoadFieldInstr::Canonicalize to replace get:length
on guarded lists with a constant.

[Florian Schneider, 2013-08-28 12:34:41]

LGTM!

[srdjan, 2013-08-28 15:15:07]

LGTM with comment.

https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1373: const Field* field =
load_array->field();
'field' can be null, do you need to check?

[Cutch, 2013-08-29 07:23:26]

https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.cc:1373: const Field* field =
load_array->field();
On 2013/08/28 15:15:08, srdjan wrote:
> 'field' can be null, do you need to check?

Done. Conceptual/context question: What does a LoadFieldInstr with a NULL field
indicate?

[Florian Schneider, 2013-08-29 08:35:15]

On 2013/08/29 07:23:26, Cutch wrote:
>
https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
> File runtime/vm/intermediate_language.cc (right):
> 
>
https://codereview.chromium.org/23584002/diff/11001/runtime/vm/intermediate_l...
> runtime/vm/intermediate_language.cc:1373: const Field* field =
> load_array->field();
> On 2013/08/28 15:15:08, srdjan wrote:
> > 'field' can be null, do you need to check?
> 
> Done. Conceptual/context question: What does a LoadFieldInstr with a NULL
field
> indicate?

It could be a load from a VM-internal field where we don't create Field object.
Also, loads from the context (captured locals) are also represented as plain
LoadFieldInstr.

[Cutch, 2013-09-09 05:48:12]

Committed patchset #5 manually as r27280 (presubmit successful).