Hook ByteArrayViewLoad operations into polymorphic inliner

This CL enables the polymorphic inliner to inline loads on typed array views.

It also contains a small cleanup so that the signatures of BuildByteArrayViewLoad and BuildByteArrayViewStore match.

R=fschneider@google.com

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

[Cutch, 2013-10-07 23:49:50]

https://codereview.chromium.org/26191002/diff/6001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/26191002/diff/6001/runtime/lib/typed_data.dar...
runtime/lib/typed_data.dart:698: int _getUint8(int offsetInBytes) native
"TypedData_GetUint8";
I have to override _getUint8 in _Uint8Array and _ExternalUint8Array (see below)
so that the IC data has more than one target. Without this change the optimizer
and polymorphic inliner think the call site has a single target (correctly) but
the target itself determines the class id and does the right thing.

The problem is that we have ~18 TypedArray classes and would need 18*18 methods
(and add them all to the method recognizer— and that's just counting the load
operations) to support a load of any type from a view of any type. We could
support few but common configurations.

[srdjan, 2013-10-09 17:14:11]

Maybe we could adapt TryRepplaceXXX functions to optimize for single target,
multiple cid's?

[Florian Schneider, 2013-10-10 13:32:44]

On 2013/10/09 17:14:11, srdjan wrote:
> Maybe we could adapt TryRepplaceXXX functions to optimize for single target,
> multiple cid's?

Handling it in FlowGraphOptimizer::TryInlineInstanceMethod would require to
build the dispatch on the cid that the PolymorphicInliner normally builds. The
problem is that those methods (ByteArrayBaseGetInt8) do on the receiver type
internally. I think this would be pretty complicated as well to do in the
optimizer.

On possibility would be to handle recognized methods differently in the
PolymorphicInliner: e.g. Don't try to share one target body, but invoke
TryInlineRecognizedMethod for each cid separately for those recognized methods
that dispatch on the receiver types in their implementation. This way
TryInlineRecognizedMethod can produce a target specialized for the cid
corresponding to each polymorphic variant. Does that make sense?

[Cutch, 2013-10-10 15:02:58]

On 2013/10/10 13:32:44, Florian Schneider wrote:
> On possibility would be to handle recognized methods differently in the
> PolymorphicInliner: e.g. Don't try to share one target body, but invoke
> TryInlineRecognizedMethod for each cid separately for those recognized methods
> that dispatch on the receiver types in their implementation. This way
> TryInlineRecognizedMethod can produce a target specialized for the cid
> corresponding to each polymorphic variant. Does that make sense?

Last weekend I went down this road but stopped because I wasn't sure the ideal
way to handle it. this work can be seen in Patch Set 1 (PS1). It marks all
methods in the MethodRecognizer as "always inline" and adjusts the optimizer and
inliner to respect that. The criteria in PS1 may be too broad but I can easily
make a pared down list of methods which we should always inline. Could you PTAL
a look at my changes in PS1? Understand that the always_inline stuff was hacked
in as an experiment.

John

[Cutch, 2013-10-22 03:46:16]

This is ready for review.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_inl...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:367: bool TryInlineRecognizedMethod(intptr_t
receiver_cid, const Function& target);
Because we now inline single target (but multiple receiver id) functions, we
must pass the receiver_cid down.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:1037:
!MethodRecognizer::PolymorphicTarget(target)) {
If a method is in the polymorphic target list, having the same target doesn't
indicate that it is a duplicate.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_inl...
runtime/vm/flow_graph_inliner.cc:1419: const intptr_t receiver_cid =
variants_[var_idx].cid;
Fetch the receiver_cid and pass it all the way down into the optimizer
TryInlineRecognizedMethod.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1079: case
MethodRecognizer::kByteArrayBaseGetInt8:
We now support polymorphic inlining loads on byte array views.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:2072: return BuildByteArrayViewLoad(call,
kTypedDataUint8ArrayCid);
Cleanup so that signatures of BuildByteArrayViewLoad and BuildByteArrayViewStore
match.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.h (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.h:137: bool
BuildByteArrayViewLoad(InstanceCallInstr* call,
Minor cleanup here: Make signatures for BuildByteArrayViewLoad and
BuildByteArrayViewStore match.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:183: #define POLYMORPHC_TARGET_LIST(V)       
                                      \
These methods internally dispatch on the receiver id.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language.h:208: static bool PolymorphicTarget(const
Function& function);
Returns true if the target is in the above POLYMORPHIC_TARGET_LIST. This is used
in the polymorphic inliner where it forces the above methods to be inlined
despite only having one target.

[Florian Schneider, 2013-10-22 15:57:33]

LGTM.

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1113: case
MethodRecognizer::kByteArrayBaseGetFloat32x4:
Do you need to check for ShouldInlineSimd() here?

[Cutch, 2013-10-23 09:19:00]

Committed patchset #9 manually as r29066 (presubmit successful).

[Cutch, 2013-10-23 09:19:52]

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/26191002/diff/84001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:1113: case
MethodRecognizer::kByteArrayBaseGetFloat32x4:
On 2013/10/22 15:57:33, Florian Schneider wrote:
> Do you need to check for ShouldInlineSimd() here?

Done.