harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gregory Shimansky <gshiman...@apache.org>
Subject Re: [drlvm][jit][jet][x86_64] Lazy resolution for JET
Date Fri, 25 Jan 2008 00:07:49 GMT
On 24 January 2008 Gregory Shimansky wrote:
> Mikhail Fursov said the following on 24.01.2008 13:11:
> > Gregory, here are the answers to your questions:
> >
> > On Jan 23, 2008 8:02 PM, Gregory Shimansky <gshimansky@apache.org> wrote:
> >> The code that contained calls for lazy resolution helpers have
> >> unbalanced runlock calls when CallSig is not locked, so assertions fail
> >> that register is not locked.
> >
> > The patch is correct.
> >
> >> In call_va function there was no implementation for passing i64
> >> arguments. I also added a 64-bit version for passing pointer arguments.
> >
> > this is correct.
> Can you also take a look at my changes in gen_args? Are they correct too?
> >> Patch also sets lazy resolution mode as default.
> >>
> >> Now that I made these fixes some Java code is executed, but Hello World
> >> doesn't work yet. The problem is with calling
> >> invokevirtual/invokeinterface resolution helpers. They require "thiz"
> >> argument as a pointer to the object. The "thiz" value is locked in the
> >> register at the beginning of gen_invoke method and is unlocked only at
> >> the end of it. But it so happens that "thiz" may be locked in some
> >> register that is
> >
> > required to pass arguments to the resolution helper because CallSig is
> >
> >> locked only before the helper is called. In this case the value of this
> >> register is overwritten by the arguments of the helper because this
> >> register is needed for arguments, and therefore the object argument of
> >> the helper is corrupted.
> >
> > The lock of 'thiz' at the beginning of the method is an old optimization.
> > It is not correct today.
> > It does not related to lazy resolution mode and theoretically can cause a
> > failure for CCONV_MANAGED calls too.
> > IMO the best solution here is to move global rlock(thiz) into local
> > places that require ''thiz' to be on a register.
> I was going to do exactly this, but I am a but uncertain if the
> thiz_depth is the same throughout the whole body of the method. If it is
> the same, then locking of "thiz" can be easily moved into each path.
> > Another problem here: in lazy resolution mode we need to 'vpark' all
> > registers for CCONV_HELPERS calls too.
> Hmm... I am not yet experienced with JIT well enough to understand this.
>   I have seen vpark calls in many places but I don't know what it does
> and why it is inserted. Could you please explain in a few words? BTW
> vpark is done at the beginning of the gen_invoke for the CallSig of the
> method.
> >> I think it is necessary to split this method, it is quite complicated
> >> and has too many branches to follow so that lazy resolution path and
> >> normal call path are compiled independently. This will allow to lock
> >> "thiz" after the helper signature is locked. But I am not very familiar
> >> with JET, so I am not sure if reordering the locking is the only
> >> possible way to fix this bug. Could someone from JIT team enlighten me?
> >
> > By splitting the method you have to repeat all the
> > preparation/finalization for a call in both versions. It's about a half
> > size of the current method. What do you think by moving the <do_smthX>
> > from
> You are right, it is probably not easy to split this method. I won't try
> it anyway because I don't want to make any intrusive changes in JIT yet.
> > if(meth==NULL) {
> >    <do_smth1>
> > } else if (opcod == OPCODE_INVOKEINTERFACE) {
> >   <do_smth2>
> > }...
> >
> > into separate methods?
> This was my thought to separate lazy and non-lazy paths. I don't insist
> on this.

BTW I've made some progress in this area. I made Hello World to work!!! This 
is quite a good result because HW actually does a pretty big number of 
resolutions on class library startup.

With all of my limited understanding of what's going on inside of JET I 
rewrote gen_invoke according to Mikhail's recommendation, so I moved 
rlock(thiz) into the branches where it is needed. This happened not to be 
trivial because gen_stack_args that was executed after global rlock(thiz) 
made rearrangements on the stack, and so it had to be moved inside of the 
branches as well. In addition gen_check_null needed "thiz", so all of this 
stuff had to be moved in there as well.

Alternatively I think it could be possible to leave thiz initialization where 
it was and initialize it with vstack(thiz_depth, false) (note the difference, 
on 2nd argument), and allocate a register in the branches where a register is 
needed in case vstack(thiz_depth, false) for thiz produced memory value. But 
premature optimizations are often harmful. I'm going to get lazy resolution 
to be completely working first, then try to clean up the code.

To make understanding of the changes easier I attached two new patches to 
HARMONY-5322. One with my changes, another one contains the whole new version 
of gen_invoke and the original commented with #if 0.

Eclipse didn't start however crashing on rth_checkcast_withresolve with "obj" 
argument corrupted. It doesn't seem to be related to my changes in gen_invoke 
(although who knows), maybe there are still some bugs to be found. Any 
recommendations are welcome!


View raw message