Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894953002:
Support inlining of calls where type arguments are passed to generic functions. (Closed)
|
Created:
3 years, 7 months ago by regis Modified:
3 years, 5 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSupport inlining of calls where type arguments are passed to generic functions.
R=vegorov@google.com
Committed: https://github.com/dart-lang/sdk/commit/b5170a8b250a367e15f4903638eb87cdbc208273
Patch Set 1 #
Total comments: 6
Patch Set 2 : work in progress #
Total comments: 2
Patch Set 3 : work in progress #Patch Set 4 : sync and work in progress #Patch Set 5 : prevent spill slot trashing #Patch Set 6 : sync #Patch Set 7 : sync #Patch Set 8 : sync #Patch Set 9 : work in progress #Patch Set 10 : work in progress #
Total comments: 2
Patch Set 11 : work in progress #Patch Set 12 : Merge branch 'master' into slave #
Total comments: 20
Patch Set 13 : address review comments #
Total comments: 8
Patch Set 14 : address review comments #
Messages
Total messages: 21 (2 generated)
regis@google.com changed reviewers: + rmacnak@google.com, vegorov@google.com
Btw, this is more a RFC than a real code review, because I am not familiar with the flow graph allocator. So feel free to shoot this proposal down and suggest alternative approaches. Until I implement the code in a generic function's prolog storing the passed-in type arguments, things will remain fuzzy. Thanks, Regis
https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:683: if ((flow_graph_->FunctionTypeArgsVar() != NULL) && Is this a mutable variable? I thought it's just an incoming argument which can't be mutated. https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:1031: if (i == type_args_env_index) { I think a better way would be to mimic what we do for incomming closure context and use a special marker in the environment. For closure context we use CurrentContextInstr (which is confusingly named for historical reasons) - and register allocator / inliner hooks it up with a correct value. Maybe what we should do is rename CurrentContextInstr to SpecialParameter(kind) and have SpecialParameter(kContext) for context SpecialParameter(kTypeArguments) for arguments then we can handle both type arguments and context here uniformly and you will not need to thread inlining_type_args into here. Instead you hook it up in the inliner itself: search for AsCurrentContext to see how the context is hooked up.
https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:683: if ((flow_graph_->FunctionTypeArgsVar() != NULL) && On 2017/05/22 11:23:21, Vyacheslav Egorov (Google) wrote: > Is this a mutable variable? > > I thought it's just an incoming argument which can't be mutated. It is invariable with one exception: if the generic function is nested in a parent generic function, the type argument vector of the parent is prepended to the passed-in type argument vector. This is done only once on entry. I plan to do this by a runtime call. Would this prevent using a SpecialParameterInstr(kind) as you suggest below? https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:1031: if (i == type_args_env_index) { On 2017/05/22 11:23:21, Vyacheslav Egorov (Google) wrote: > I think a better way would be to mimic what we do for incomming closure context > and use a special marker in the environment. > > For closure context we use CurrentContextInstr (which is confusingly named for > historical reasons) - and register allocator / inliner hooks it up with a > correct value. > > Maybe what we should do is rename CurrentContextInstr to SpecialParameter(kind) > and have > > SpecialParameter(kContext) for context > SpecialParameter(kTypeArguments) for arguments > > then we can handle both type arguments and context here uniformly and you will > not need to thread inlining_type_args into here. Instead you hook it up in the > inliner itself: search for AsCurrentContext to see how the context is hooked up. > > > Thank you for the suggestion. Let me try that.
https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:683: if ((flow_graph_->FunctionTypeArgsVar() != NULL) && On 2017/05/22 17:51:08, regis wrote: > On 2017/05/22 11:23:21, Vyacheslav Egorov (Google) wrote: > > Is this a mutable variable? > > > > I thought it's just an incoming argument which can't be mutated. > > It is invariable with one exception: if the generic function is nested in a > parent generic function, the type argument vector of the parent is prepended to > the passed-in type argument vector. This is done only once on entry. I plan to > do this by a runtime call. Would this prevent using a > SpecialParameterInstr(kind) as you suggest below? I suppose I could use the SpecialParameterInstr only if the generic function does not have a generic parent. And I could bail out of inlining it if it has one.
https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... runtime/vm/flow_graph.cc:683: if ((flow_graph_->FunctionTypeArgsVar() != NULL) && On 2017/05/22 18:02:42, regis wrote: > On 2017/05/22 17:51:08, regis wrote: > > On 2017/05/22 11:23:21, Vyacheslav Egorov (Google) wrote: > > > Is this a mutable variable? > > > > > > I thought it's just an incoming argument which can't be mutated. > > > > It is invariable with one exception: if the generic function is nested in a > > parent generic function, the type argument vector of the parent is prepended > to > > the passed-in type argument vector. This is done only once on entry. I plan to > > do this by a runtime call. Would this prevent using a > > SpecialParameterInstr(kind) as you suggest below? > > I suppose I could use the SpecialParameterInstr only if the generic function > does not have a generic parent. And I could bail out of inlining it if it has > one. I think there is no problem. I think the best way to express the second case (when you need to prepend something) is by emitting explicit store-local - then a lot of things will work out of the box, e.g. 1) First case: no nesting. Unopt Graph: // no special code in the graph itself. // function entry point handles copying of type arguments from // stack slot before the receiver into :function_type_args variable // (just what we do with context). All uses just load this variable LoadLocal(:function_type_args) USE(...) Opt Graph: // no special code in the graph, // initial declarations in B0 contain special instruction // SpecialParameterInstr(kTypeArgs). // SSA renaming handles the rest. B0 { v42 <- SpecialParameter(kTypeArgs) } ... USE(v42) 2) Second case: function is nested, requires prepending on entry. Unopt Graph: // function entry point handles copying of type arguments from // stack slot before the receiver into :function_type_args variable // (just what we do with context). Then there is code in B1 doing // prepending - then result is stored in the local B1: LoadLocal(:function_type_args) PushArgument() StaticCall(Object._prependTypeArguments, ...) StoreLocal(:function_type_args) ... somewhere later ... LoadLocal(:function_type_args) USE(...) Optimized Graph: // SSA renaming will handle everything. B0 { v42 <- SpecialParameter(kTypeArgs) } B1: PushArgument(v42) v43 <- StaticCall(Object._prependTypeArguments, v42) ... USE(v43) // SSA renaming handled it
On 2017/05/22 18:39:44, Vyacheslav Egorov (Google) wrote: > https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc > File runtime/vm/flow_graph.cc (right): > > https://codereview.chromium.org/2894953002/diff/1/runtime/vm/flow_graph.cc#ne... > runtime/vm/flow_graph.cc:683: if ((flow_graph_->FunctionTypeArgsVar() != NULL) > && > On 2017/05/22 18:02:42, regis wrote: > > On 2017/05/22 17:51:08, regis wrote: > > > On 2017/05/22 11:23:21, Vyacheslav Egorov (Google) wrote: > > > > Is this a mutable variable? > > > > > > > > I thought it's just an incoming argument which can't be mutated. > > > > > > It is invariable with one exception: if the generic function is nested in a > > > parent generic function, the type argument vector of the parent is prepended > > to > > > the passed-in type argument vector. This is done only once on entry. I plan > to > > > do this by a runtime call. Would this prevent using a > > > SpecialParameterInstr(kind) as you suggest below? > > > > I suppose I could use the SpecialParameterInstr only if the generic function > > does not have a generic parent. And I could bail out of inlining it if it has > > one. > > I think there is no problem. I think the best way to express the second case > (when you need to prepend something) is by emitting explicit store-local - then > a lot of things will work out of the box, e.g. > > 1) First case: no nesting. > > Unopt Graph: > // no special code in the graph itself. > // function entry point handles copying of type arguments from > // stack slot before the receiver into :function_type_args variable > // (just what we do with context). All uses just load this variable > LoadLocal(:function_type_args) > USE(...) > > Opt Graph: > // no special code in the graph, > // initial declarations in B0 contain special instruction > // SpecialParameterInstr(kTypeArgs). > // SSA renaming handles the rest. > B0 { > v42 <- SpecialParameter(kTypeArgs) > } > ... > USE(v42) > > 2) Second case: function is nested, requires prepending on entry. > > Unopt Graph: > // function entry point handles copying of type arguments from > // stack slot before the receiver into :function_type_args variable > // (just what we do with context). Then there is code in B1 doing > // prepending - then result is stored in the local > B1: > LoadLocal(:function_type_args) > PushArgument() > StaticCall(Object._prependTypeArguments, ...) > StoreLocal(:function_type_args) > ... somewhere later ... > LoadLocal(:function_type_args) > USE(...) > > Optimized Graph: > // SSA renaming will handle everything. > B0 { > v42 <- SpecialParameter(kTypeArgs) > } > B1: > PushArgument(v42) > v43 <- StaticCall(Object._prependTypeArguments, v42) > ... > USE(v43) // SSA renaming handled it Slava, This cl has grown quite a bit. I am uploading it so you can have another look. I followed your suggestions regarding a new SpecialParameterInstr, but not everything is working yet. Unoptimized code seems to work (except for DBC), but not optimized code. I see problems both with --use-inlining and --no-use-inlining. For example, I hit an assert in the graph allocator that a definition has no representation. So I definitely still have serious issues, and lots of todos in the source. My plan is to move the parts that are working and that are required for unoptimized code in a separate cl. I will also add support for DBC. I will then come back to this cl after it is much reduced. In case you want to play with it, you need to pass --reify-generic-functions. I use the small test below that is complicated enough to blow things off (with --optimization-counter-threshold=10): foo<T>() { print(T); bar<V>() { print(V); } bar<T>(); } main() { for (int i = 0; i < 20; i++) { foo<int>(); } } Thanks, Regis
https://codereview.chromium.org/2894953002/diff/20001/runtime/vm/flow_graph_i... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/20001/runtime/vm/flow_graph_i... runtime/vm/flow_graph_inliner.cc:1154: for (intptr_t i = 0; i < arguments->length(); ++i) { Hi Regis! I briefly looked at the crash: it seems to happen because one of the parameters of an inlined function was not correctly replaced with an actual value leading to a malformed graph. I think it is not replaced because sometimes we arrive to this loop with arguments->length() < call_data->parameter_stubs->length which leaves all parameter stubs with indices i = arguments->length(), ..., (call_data->parameter_stubs->length - 1) in the graph. I did not debug why exactly it happens.
https://codereview.chromium.org/2894953002/diff/20001/runtime/vm/flow_graph_i... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/20001/runtime/vm/flow_graph_i... runtime/vm/flow_graph_inliner.cc:1154: for (intptr_t i = 0; i < arguments->length(); ++i) { On 2017/06/12 10:52:11, Vyacheslav Egorov (Google) wrote: > Hi Regis! > > I briefly looked at the crash: it seems to happen because one of the parameters > of an inlined function was not correctly replaced with an actual value leading > to a malformed graph. > > I think it is not replaced because sometimes we arrive to this loop with > > arguments->length() < call_data->parameter_stubs->length > > which leaves all parameter stubs with indices i = arguments->length(), ..., > (call_data->parameter_stubs->length - 1) in the graph. > > I did not debug why exactly it happens. Thanks a lot Slava! Indeed, these arrays are out of sync, because I create a parameter stub to accept an optional type argument vector, which is not always provided by the caller of the inlined call. I updated the cl to fix this case. However, I hit another assert further down the line. I will stick to the plan of setting this aside for a while and commit the working parts in order to reduce this cl. Thanks again! Regis
Hi Regis!
I think it is a great idea to split this into several CLs.
I looked at the next crash (one in SSA translation). The reason for this crash
is that changes in the flow_graph_builder don't obey the stack discipline.
Unoptimized IR is essentially the stack machine, so you must consume values in
the same order as they are produced, e.g.
t0 <- Def
t1 <- Def
t0 <- Use(t0, t1)
is a valid unoptimized IR.
t0 <- Def
t1 <- Def
Use(t0)
Use(t1)
is not a valid unoptimized IR.
The code that violates stack discipline is in the flow_graph_builder.cc
Value* type_args_val =
Bind(BuildLoadLocal(*temp_local, node->token_pos()));
if (parent_type_args_var != NULL) {
ASSERT(parent_type_args_var->owner() != scope);
// Call the runtime to concatenate both vectors.
Value* parent_type_args_val =
Bind(BuildLoadLocal(*parent_type_args_var, node->token_pos()));
ZoneGrowableArray<PushArgumentInstr*>* arguments =
new (Z) ZoneGrowableArray<PushArgumentInstr*>(3);
arguments->Add(PushArgument(type_args_val));
arguments->Add(PushArgument(parent_type_args_val));
Here you emit:
t0 <- LoadLocal
t1 <- LoadLocal
PushArgument t0
PushArgument t1
Which is not valid, and should be:
t0 <- LoadLocal
PushArgument t0
t0 <- LoadLocal
PushArgument t1
[Unoptimized IR sometimes ends up working even if it does not follow the stack
discipline because some IR instructions are no-ops in the unopt mode, e.g.
PushArgument does nothing because values is already on the stack, so code like
t0 <- Def()
t1 <- Def()
PushArgument t0
PushArgument t1
ends up working even though it is *not* valid IR and breaks SSA translation
later.]
Hope this helps,
Slava.
On 2017/06/13 12:03:46, Vyacheslav Egorov (Google) wrote:
> Hi Regis!
>
> I think it is a great idea to split this into several CLs.
>
> I looked at the next crash (one in SSA translation). The reason for this crash
> is that changes in the flow_graph_builder don't obey the stack discipline.
> Unoptimized IR is essentially the stack machine, so you must consume values in
> the same order as they are produced, e.g.
>
> t0 <- Def
> t1 <- Def
> t0 <- Use(t0, t1)
>
> is a valid unoptimized IR.
>
> t0 <- Def
> t1 <- Def
> Use(t0)
> Use(t1)
>
> is not a valid unoptimized IR.
>
> The code that violates stack discipline is in the flow_graph_builder.cc
>
> Value* type_args_val =
> Bind(BuildLoadLocal(*temp_local, node->token_pos()));
> if (parent_type_args_var != NULL) {
> ASSERT(parent_type_args_var->owner() != scope);
> // Call the runtime to concatenate both vectors.
> Value* parent_type_args_val =
> Bind(BuildLoadLocal(*parent_type_args_var, node->token_pos()));
> ZoneGrowableArray<PushArgumentInstr*>* arguments =
> new (Z) ZoneGrowableArray<PushArgumentInstr*>(3);
> arguments->Add(PushArgument(type_args_val));
> arguments->Add(PushArgument(parent_type_args_val));
>
> Here you emit:
>
> t0 <- LoadLocal
> t1 <- LoadLocal
> PushArgument t0
> PushArgument t1
>
> Which is not valid, and should be:
>
> t0 <- LoadLocal
> PushArgument t0
> t0 <- LoadLocal
> PushArgument t1
>
> [Unoptimized IR sometimes ends up working even if it does not follow the stack
> discipline because some IR instructions are no-ops in the unopt mode, e.g.
> PushArgument does nothing because values is already on the stack, so code like
>
> t0 <- Def()
> t1 <- Def()
> PushArgument t0
> PushArgument t1
>
> ends up working even though it is *not* valid IR and breaks SSA translation
> later.]
>
> Hope this helps,
> Slava.
Once again, thank you Slava for debugging my code and providing a detailed
explanation.
The fixed code in flow_graph_builder.cc will end up in a different cl and I'll
come back to this cl later.
Thanks,
Regis
On 2017/06/13 15:55:22, regis wrote:
> On 2017/06/13 12:03:46, Vyacheslav Egorov (Google) wrote:
> > Hi Regis!
> >
> > I think it is a great idea to split this into several CLs.
> >
> > I looked at the next crash (one in SSA translation). The reason for this
crash
> > is that changes in the flow_graph_builder don't obey the stack discipline.
> > Unoptimized IR is essentially the stack machine, so you must consume values
in
> > the same order as they are produced, e.g.
> >
> > t0 <- Def
> > t1 <- Def
> > t0 <- Use(t0, t1)
> >
> > is a valid unoptimized IR.
> >
> > t0 <- Def
> > t1 <- Def
> > Use(t0)
> > Use(t1)
> >
> > is not a valid unoptimized IR.
> >
> > The code that violates stack discipline is in the flow_graph_builder.cc
> >
> > Value* type_args_val =
> > Bind(BuildLoadLocal(*temp_local, node->token_pos()));
> > if (parent_type_args_var != NULL) {
> > ASSERT(parent_type_args_var->owner() != scope);
> > // Call the runtime to concatenate both vectors.
> > Value* parent_type_args_val =
> > Bind(BuildLoadLocal(*parent_type_args_var, node->token_pos()));
> > ZoneGrowableArray<PushArgumentInstr*>* arguments =
> > new (Z) ZoneGrowableArray<PushArgumentInstr*>(3);
> > arguments->Add(PushArgument(type_args_val));
> > arguments->Add(PushArgument(parent_type_args_val));
> >
> > Here you emit:
> >
> > t0 <- LoadLocal
> > t1 <- LoadLocal
> > PushArgument t0
> > PushArgument t1
> >
> > Which is not valid, and should be:
> >
> > t0 <- LoadLocal
> > PushArgument t0
> > t0 <- LoadLocal
> > PushArgument t1
> >
> > [Unoptimized IR sometimes ends up working even if it does not follow the
stack
> > discipline because some IR instructions are no-ops in the unopt mode, e.g.
> > PushArgument does nothing because values is already on the stack, so code
like
> >
> > t0 <- Def()
> > t1 <- Def()
> > PushArgument t0
> > PushArgument t1
> >
> > ends up working even though it is *not* valid IR and breaks SSA translation
> > later.]
> >
> > Hope this helps,
> > Slava.
>
> Once again, thank you Slava for debugging my code and providing a detailed
> explanation.
> The fixed code in flow_graph_builder.cc will end up in a different cl and I'll
> come back to this cl later.
>
> Thanks,
> Regis
Hi Slava,
This cl has been reduced to the inlining/optimizing parts only.
It mostly works, but background compilation still hits the assert in
graph_allocator.cc:395
ASSERT(rep != kNoRepresentation);
This test will hit it (not every time):
out/DebugX64/dart --reify-generic-functions --packages=.packages
tests/corelib/queue_test.dart
I also have a couple of TODO with open questions.
Thanks for having a look, if you have time.
Regis
https://codereview.chromium.org/2894953002/diff/180001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/180001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:1589: // Replace parameter stubs and constants. Replace the receiver argument I think that parameter stubs replacement that happens here needs to match the logic in the InlineCall on line 1147. Ideally we want to move this logic into a shared function if possible. I don't think it is very different. I can see a lot of duplicated code between here and monomorphic InlineCall(...).
Thank you very much again for your help, Slava! There is more work to do in the precompiler, kernel, DBC, etc... Also, the Zone API needs fixing (floitsch has a pending cl). But I think this CL is now in a shape decent enough to be committed. Please, let me know what you think. Thanks, Regis https://codereview.chromium.org/2894953002/diff/180001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/180001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:1589: // Replace parameter stubs and constants. Replace the receiver argument On 2017/06/28 14:56:02, Vyacheslav Egorov (Google) wrote: > I think that parameter stubs replacement that happens here needs to match the > logic in the InlineCall on line 1147. > > Ideally we want to move this logic into a shared function if possible. I don't > think it is very different. I can see a lot of duplicated code between here and > monomorphic InlineCall(...). > That was it! Thanks! I moved this code into a shared function as you suggested.
initial comments. See comment in flow_graph_inliner.cc for a thing that is causing the crash. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.... runtime/vm/flow_graph.cc:1009: // If inlined_type_args_param == 1, then (*inlining_parameters)[0] I think this comment is misplaced - it confused me because it is talking about "not adding it to env[0]" but next line is exactly env.Add(...). The comment probably needs to go up to the line number 1006 where we do inlined_type_args_param + i https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.... runtime/vm/flow_graph.cc:1035: if (inlined_type_args_param > 0) { maybe Definition* defn; if (inlining_parameters == NULL) { defn = new SpecialParameterInstr(...); } else { defn = (*inlining_parameters)[0]; } AllocateSSAIndexes(defn); AddToInitialDefinitions(defn); env.Add(defn); https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_allocator.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:707: ASSERT(slot_index >= 0); // TODO(regis): Can we encounter a type args here? I don't think we can encounter type args here. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:562: Definition* stub = (*call_data->parameter_stubs)[first_arg_stub_index]; I think this is the problematic place. This should probably be first_arg_stub_index + i Consider refactoring code to remove the duplication of the last two lines: Definition* defn = NULL; if (is_polymorphic && (i == ...)) { ... defn = redefinition; } else if (value != NULL) { defn = value->definition(); } if (defn != NULL) { call_data->parameter_stubs->At(first_arg_stub_index + i)->ReplaceUsesWith(...) } https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:593: ASSERT(!is_polymorphic); maybe leave a comment here that we currently don't support polymorphic inlining of closure calls (we used to support that back in the days when there was a class per closure, that's why inliner contained some code to deal with it) https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_type_propagator.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_type_propagator.cc:984: intptr_t cid = kind() == kContext ? kContextCid : kTypeArgumentsCid; maybe make a switch here? Then we would know which places to update if we add a new kind of special parameter. I think in general: switch (kind()) { case A: case B: } is preferred to if (kind() == A) { } else { ASSERT(kind() == B); } because in the first pattern compiler will issue a compile time error if switch is not exhaustive. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... File runtime/vm/intermediate_language.h (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2782: // (normally held in a register) or the type arguments of a generic function. "normally held in a register" part of the comment is obsolete. Historically we used to have a dedicated register CTX for the context, but Florian has long time ago refactored backend to use a normal variable for closure context. (We still use CTX to pass context value between function prologue and function body in optimized code, but otherwise it is a completely free register used for other things). https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2783: // This is a computation, not a value, because it's mutable. "This is computation, not a value" is an obsolete comment from ancient times. We used to have very sophisticated IR distinction between computations and values - but that all got refactored away in very early days of this JIT. Now this sentence makes no sense, definition can't be mutable. It is an SSA value which does not change once it is computed. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2797: virtual bool AttributesEqual(Instruction* other) const { return true; } this needs to be: return kind() == other->AsSpecialParameter()->kind(); https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/jit_optimiz... File runtime/vm/jit_optimizer.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/jit_optimiz... runtime/vm/jit_optimizer.cc:104: call->ArgumentCount() - receiver_index); I think you can use call->ArgumentCountWithoutTypeArgs() here.
Thank you Slava! The error is gone! PTAL https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.... runtime/vm/flow_graph.cc:1009: // If inlined_type_args_param == 1, then (*inlining_parameters)[0] On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > I think this comment is misplaced - it confused me because it is talking about > "not adding it to env[0]" but next line is exactly env.Add(...). > > The comment probably needs to go up to the line number 1006 where we do > inlined_type_args_param + i > Done. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph.... runtime/vm/flow_graph.cc:1035: if (inlined_type_args_param > 0) { On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > maybe > > Definition* defn; > if (inlining_parameters == NULL) { > defn = new SpecialParameterInstr(...); > } else { > defn = (*inlining_parameters)[0]; > } > AllocateSSAIndexes(defn); > AddToInitialDefinitions(defn); > env.Add(defn); Done. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_allocator.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:707: ASSERT(slot_index >= 0); // TODO(regis): Can we encounter a type args here? On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > I don't think we can encounter type args here. Acknowledged. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:562: Definition* stub = (*call_data->parameter_stubs)[first_arg_stub_index]; On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > I think this is the problematic place. This should probably be > first_arg_stub_index + i > > Consider refactoring code to remove the duplication of the last two lines: > > Definition* defn = NULL; > if (is_polymorphic && (i == ...)) { > ... > defn = redefinition; > } else if (value != NULL) { > defn = value->definition(); > } > > if (defn != NULL) { > call_data->parameter_stubs->At(first_arg_stub_index + i)->ReplaceUsesWith(...) > } Good catch! If first_arg_stub_index is 1, i has to be 0 (+i makes no difference), but first_arg_stub_index can be 0 while i is 1 (bug!!). I refactored the code as suggested. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:593: ASSERT(!is_polymorphic); On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > maybe leave a comment here that we currently don't support polymorphic inlining > of closure calls (we used to support that back in the days when there was a > class per closure, that's why inliner contained some code to deal with it) Done. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_type_propagator.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/flow_graph_... runtime/vm/flow_graph_type_propagator.cc:984: intptr_t cid = kind() == kContext ? kContextCid : kTypeArgumentsCid; On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > maybe make a switch here? > > Then we would know which places to update if we add a new kind of special > parameter. > > I think in general: > > switch (kind()) { > case A: > case B: > } > > is preferred to if (kind() == A) { } else { ASSERT(kind() == B); } > > because in the first pattern compiler will issue a compile time error if switch > is not exhaustive. Done. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... File runtime/vm/intermediate_language.h (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2782: // (normally held in a register) or the type arguments of a generic function. On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > "normally held in a register" part of the comment is obsolete. > > Historically we used to have a dedicated register CTX for the context, but > Florian has long time ago refactored backend to use a normal variable for > closure context. (We still use CTX to pass context value between function > prologue and function body in optimized code, but otherwise it is a completely > free register used for other things). > Updated comment. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2783: // This is a computation, not a value, because it's mutable. On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > "This is computation, not a value" is an obsolete comment from ancient times. We > used to have very sophisticated IR distinction between computations and values - > but that all got refactored away in very early days of this JIT. Now this > sentence makes no sense, definition can't be mutable. It is an SSA value which > does not change once it is computed. Removed comment. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/intermediat... runtime/vm/intermediate_language.h:2797: virtual bool AttributesEqual(Instruction* other) const { return true; } On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > this needs to be: > > return kind() == other->AsSpecialParameter()->kind(); Done. https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/jit_optimiz... File runtime/vm/jit_optimizer.cc (right): https://codereview.chromium.org/2894953002/diff/220001/runtime/vm/jit_optimiz... runtime/vm/jit_optimizer.cc:104: call->ArgumentCount() - receiver_index); On 2017/07/03 16:15:43, Vyacheslav Egorov (Google) wrote: > I think you can use call->ArgumentCountWithoutTypeArgs() here. Done.
lgtm https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_allocator.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:773: if ((defn->IsParameter() || is_type_args_param) && I think this can be rewritten to if (range->spill_slot().IsStackSlot() && range->spill_slot().stack_index() >= 0) { // ... } then you don't need to have is_type_args_param variable. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:775: // Parameters above the frame pointer consume spill slots and are marked You can replace this comment with something like: // On entry to the function range is stored on the stack above the FP // in the same space which is used for spill slots. Update // spill slot state to reflect that and prevent register allocator // from reusing this space as a spill slot. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:529: if (!is_polymorphic) { Maybe this case needs to be kept in CallSiteInliner::InlineCall() given that this code is not shared? If you decide to do that, then a better name for InlineCall is probably ReplaceParameterStubs, because that's what it does. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_type_propagator.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_type_propagator.cc:985: switch (kind()) { I would structure this a bit differently: switch (kind()) { case kContext: return CompileType::FromCid(kContextCid); case kTypeArguments: return CompileType::FromCid(kTypeArgumentsCid); } UNREACHABLE(); return CompileType::Dynamic(); The reason for that is that having default: disables compile time checking for exhaustiveness of the switch - which was my primary reason for recommending the switch.
Thank you! https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_allocator.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:773: if ((defn->IsParameter() || is_type_args_param) && On 2017/07/06 10:31:56, Vyacheslav Egorov (Google) wrote: > I think this can be rewritten to > > if (range->spill_slot().IsStackSlot() && range->spill_slot().stack_index() >= 0) > { > // ... > } > > then you don't need to have is_type_args_param variable. Done. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_allocator.cc:775: // Parameters above the frame pointer consume spill slots and are marked On 2017/07/06 10:31:57, Vyacheslav Egorov (Google) wrote: > You can replace this comment with something like: > > // On entry to the function range is stored on the stack above the FP > // in the same space which is used for spill slots. Update > // spill slot state to reflect that and prevent register allocator > // from reusing this space as a spill slot. Done. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_inliner.cc:529: if (!is_polymorphic) { On 2017/07/06 10:31:57, Vyacheslav Egorov (Google) wrote: > Maybe this case needs to be kept in CallSiteInliner::InlineCall() given that > this code is not shared? > > If you decide to do that, then a better name for InlineCall is probably > ReplaceParameterStubs, because that's what it does. If I keep CallSiteInliner::InlineCall(), it is not clear to me whether I can "plug result in the caller graph" after calling ReplaceParameterStubs. Let me commit as is and send you another cl later with this change. https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... File runtime/vm/flow_graph_type_propagator.cc (right): https://codereview.chromium.org/2894953002/diff/240001/runtime/vm/flow_graph_... runtime/vm/flow_graph_type_propagator.cc:985: switch (kind()) { On 2017/07/06 10:31:57, Vyacheslav Egorov (Google) wrote: > I would structure this a bit differently: > > switch (kind()) { > case kContext: return CompileType::FromCid(kContextCid); > case kTypeArguments: return CompileType::FromCid(kTypeArgumentsCid); > } > UNREACHABLE(); > return CompileType::Dynamic(); > > The reason for that is that having default: disables compile time checking for > exhaustiveness of the switch - which was my primary reason for recommending the > switch. Done.
Description was changed from ========== Support inlining of calls where type arguments are passed to generic functions. ========== to ========== Support inlining of calls where type arguments are passed to generic functions. R=vegorov@google.com Committed: https://github.com/dart-lang/sdk/commit/b5170a8b250a367e15f4903638eb87cdbc208273 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) manually as b5170a8b250a367e15f4903638eb87cdbc208273 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
