RFR/RFC calling convention change for value types in compiled code

[Zoltán Majó]

Hi Roland,


sorry for the delay, I was caught up with other work.

On 12/05/2016 02:28 PM, Roland Westrelin wrote:
> 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.

Oh, I missed that on the first read (but it's obvious on a second read). 
Maybe you could just add a small note to the comment saying that we need 
two elements for the long.

>>    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?

Maybe that's too much, as you would have to change all the way up to

AdapterHandlerLibrary::get_adapter()

So it's up to you if you want to change that.

>> = 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.

Thank you.

> 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.

I see. Then sig_extended is probably a good name for sig (if you decide 
to change it).

>
>> = 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.

OK, thank you for explaining.

>
>> = 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.

That sounds good to me.

Best regards,


Zoltan

>
> Roland.