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