On Wed, 18 May 2022 11:23:07 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
>> line 336:
>>
>>> 334:
>>> 335: if (callingSequence.forUpcall()) {
>>> 336: // for upcalls, recipes have a result, which we handle
>>> here
>>
>> I find this part a bit confusing. The comment speaks about recipes input and
>> outputs, hinting at the fact that downcall bindings have inputs, while
>> upcall bindings have outputs.
>> In reality, if we look at the bindings themselves, they look pretty
>> symmetric:
>>
>> * UNBOX_ADDRESS = pops an Addressable and push a long on the stack
>> * BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
>>
>> That is, in both cases it appears we have an input and an output (and the
>> binding is just the function which maps one into another).
>>
>> I think the input/output asymmetry comes in when we consider the full
>> recipes. In downcalls, for a given argument we will have the following
>> sequence:
>>
>> Binding0, Binding1, ... BindingN-1, VMStore
>>
>> While for upcalls we will have:
>>
>> VMLoad, Binding1, Binding2, ... BindingN
>>
>> So (ignoring return buffer for simplicity), for downcalls, it is obvious
>> that before we can execute Binding0, we need some kind of "input value"
>> (e.g. the value of some local). For upcalls, this is not needed, as the
>> VMLoad binding will basically do that for free.
>> When we finished executing the bindings, again, for downcalls there's
>> nothing to do, as the chain always ends with a VMStore (which stores binding
>> result into some local), while for upcalls we do need to set the output
>> value explicitly.
>>
>> In other words, it seems to me that having VMLoad and VMStore in the chains
>> of bindings to be interpreted/specialized does more harm than good here.
>> These low level bindings make sense for the VM code, as the VM needs to know
>> how to load a value from a register, or how to store a value into a
>> register. But in terms of the specializing Java code, these bindings are
>> immaterial. At the same time, the fact that we have these bindings, and that
>> they are virtualized, makes the code hard to follow, as some preparation
>> happens in `BindingSpecializer::specialize`, while some other preparation
>> happens inside `BindingSpecializer::doBindings`, as part of the virtualized
>> impl of VMLoad/VMStore. And, this virtualized implementation ends up doing
>> similar steps as what the preparation code before/after the binding
>> execution does (e.g. for downcalls/VMStore we call setOutput, while for
>> upcalls/VMLoad we call getInput).
>>
>> All I'm saying here is that, for bindings to work, we _always_ need to call
>> both getInput and setOutput - but it is easy to overlook this when looking
>> at the code, because the getInput/setOutput calls are spread across two
>> different places in the specializing code, perhaps making things more
>> asymmetric than they need to be. (I realize I'm simplifying here, and that
>> some details of the return buffer are not 100% quite as symmetric).
>
> I'm not sure if there is anything actionable here?
>
> I've thought in the past that it might be nice to have
> `GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding
> operators as well, to make the inputs/outputs more explicit in the recipe.
> But, it doesn't seem like that would make things _much_ better...
I wasn't suggesting to add more bindings. I was more suggesting to filter out
the load/store from the set of bindings (since these are virtualized anyways)
that are passed to `doBindings`. Then, before executing a set of bindings, (if
we are in downcall mode) we load the corresponding input local var. After
executing bindings (if we are in upcall mode) we store result in corresponding
var.
E.g. make the logic that load locals and store locals explicit in the
`specialize` method, rather than have parts of it execute "in disguise" as
"binding interpretation".
-------------
PR: https://git.openjdk.java.net/jdk/pull/8685
