RFR/RFC calling convention change for value types in compiled code
Roland Westrelin
rwestrel at redhat.com
Mon Dec 5 13:28:25 UTC 2016
Hi Zoltan,
Thanks for the careful review. See the comments below:
> = src/cpu/x86/vm/sharedRuntime_x86_64.cpp
>
> Don't we have one T_VOID too much here?
>
> 561 // flattened so, for instance: T_VALUETYPE T_INT T_VALUETYPE
> 562 // T_INT T_LONG T_VOID T_VOID T_VOID is a value type with a
I think it's good: one T_VOID for the T_LONG and then 2 for each
T_VALUETYPE.
> 552 static int compute_total_args_passed_int(const
> GrowableArray<SigEntry>& sig) {
>
> Maybe it would be also good to change the argument name to sig_extended
> or sig_cc so that it's more obvious that this is the signature used by
> the calling convention (and not the declared signature).
>
> So in SharedRuntime::gen_i2c_adapter, you could change
>
> 1018 int total_args_passed = compute_total_args_passed(sig);
>
> to
>
> 1018 int total_args_passed_int =
> compute_total_args_passed_int(sig_extended);
So you would change sig to sig_extended everywhere it's used?
> = src/share/vm/opto/type.hpp
>
> 1536 const TypeTuple* const _domain_sig; // Domain of inputs
> 1537 const TypeTuple* const _domain_cc; // Domain of inputs
>
> Could you please add a comment to explain what the difference between
> _domain_sig and _domain_cc is? I see that 'cc' refers to calling
> convention, but that was not obvious on the first sight. Or maybe you
> can use _domain_sig_extended instead. (Whatever you may pick, it would
> be great if it stayed consistent with the terms you use at other places,
> e.g., in the adapter generation).
>
> The comment in src/share/vm/opto/type.cpp describes very well what is
> happening, maybe you can reuse part of that text.
I copied the comment. Not sure there's more to say?
sig in sharedRuntime, domain_cc and domain_sig are not the same. sig
attempts to sum up both domain_cc and domain_sig so I'm not sure what to
do about your comment about naming consistency.
> = src/share/vm/opto/type.cpp
>
> Could you please move the extra_value_fields() and
> collect_value_fields() functions to ciValueKlass? Both functions operate
> on a ciValueKlass instance and maybe we want to use them later in some
> other context. Maybe add a short comment summarizing what those
> functions do.
I can't do that for collect_value_fields() because it fills a Type*
array which is c2 only.
> = src/share/vm/opto/callGenerator.cpp
>
> 375 // FIXME: late inlining of methods that take value type arguments is
> 376 // broken: arguments at the call are set up so fields of value type
> 377 // arguments are passed but code here expects a single argument per
> 378 // value type (a ValueTypeNode) instead.
>
> How difficult do you think it will be to fix late inlining later on?
> It's fine to do the fix later -- I'm just wondering if there is anything
> here that would cause a large difficulty for us later on (most likely
> not, but better to ask).
I'll take care of it in a later change. I don't think it's too
difficult but it's not straightforward either.
Roland.
More information about the valhalla-dev
mailing list