You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Charles Hardin <ck...@gmail.com> on 2010/01/04 23:40:46 UTC
MIPS Progress (WAS: The progress of MIPS patch work)
Tianwei,
Thanks - this is really good news. We should be able to return to
looking at the MIPS work in a few weeks.
I'll review the updates you sent and get a new patch submitted to
HARMONY-6379 with the updates
http://issues.apache.org/jira/browse/HARMONY-6379
Charles
On Sat, Jan 2, 2010 at 7:20 AM, Tianwei <ti...@gmail.com> wrote:
> Hi, Charles,
> I finally made a milestone, and get the interpreter works for
> "HelloWorld", the following is the result:
> stw@RAYS-b0f748fa:~/test$
> /home/stw/harmony/harmony-nofetch/working_vm/build/linux_mips32_gcc_debug/deploy/jdk/jre/bin/java
> -Xint HelloWorldApp
> Hello World!
>
> Please ignore the previous mail, I made a mistake to comment out the
> following line in
> interp_native_mips.cpp:
>
> case JAVA_TYPE_DOUBLE:
> case JAVA_TYPE_LONG:
>
> /* MIPS ABI requires 64 bit values to be
> naturally aligned.
> * pad here and increase array size if required.
> */
> if ((argId & 1) != 0) {
> sz++;
> arg_words2 = (uword*)ALLOC_FRAME((sz + 2)
> * sizeof(uword));
> if (!arg_words2) {
> DIE(("no memory for frame"));
> }
> memcpy(arg_words2, arg_words, argId *
> sizeof(uword));
> FREE_FRAME(arg_words);
> arg_words = arg_words2;
> argId++;
> }
> Value2 val;
> val.i64 = args[pos++].j;
> arg_words[argId++] = val.v[0].i;
> arg_words[argId++] = val.v[1].i;
> break;
>
> I comment out the padding code, this will give me the following error, for
> example:
> JNIEXPORT void JNICALL
> Java_java_util_zip_Inflater_setInputImpl (JNIEnv * env, jobject recv,
> jbyteArray buf, jint off, jint
> len,
> jlong handle)
> {
> ............
> }
>
> it has 6 arguments, in the interpreter, before call this JNI, if comment out
> the padding code, the args is:
> (gdb) p args[4]
> $88 = 512
> (gdb) p args[5]
> $85 = 10515904
> (gdb) p args[6]
> $86 = 0
>
> args[5] and args[6] are for jlong, but the MIPS ABI need to align, so we
> need to modify it as:
> args[4] =512, args[5]=0
> args[6] = 10515904, args[7]=0
> when I enable the padding code, it works.
>
> Other issues are mainly about get and set value for JAVA_TYPE_LONG, I
> believe the original patch has some problems, my fix looks like:
> Value2 val;
> val.i64 = args[pos++].j;
> arg_words[argId++] = val.v[0].i;
> arg_words[argId++] = val.v[1].i;
> break;
>
>
> summary:
> (1) the interpreter works for "HelloWorld" based on Charles's patch on my
> MIPS machines.
> (2) I only knew very little about the harmony and JVM, only use gdb and
> trace to solve the problem, I will continue to
> learn them.
> (3) next step, I will continue to test more cases and polish my code.
>
> BTW, Is there any updates for merging your patch into upstream? Since now
> I make the interpreter work, I also can help testing.
>
> Thanks.
>
> Tianwei
> On Tue, Dec 29, 2009 at 9:13 PM, Tianwei <ti...@gmail.com> wrote:
>
>> Well, after two day's debugging, the root cause is that there is some
>> problem with the parameter passing in original patch on my machine, some of
>> the code looks like:
>> --- interpreter.cpp (revision 162)
>>
>> +++ interpreter.cpp (working copy)
>>
>> @@ -1691,10 +1691,10 @@
>>
>> case VM_DATA_TYPE_INT64:
>>
>> case VM_DATA_TYPE_F8:
>>
>> {
>>
>> - double *vaddr = (double*) addr;
>>
>> - *vaddr = frame.stack.getLong(0).d;
>>
>> - frame.stack.pop(2);
>>
>> - break;
>>
>> + //double *vaddr = (double*) addr;
>>
>> + *(I_64*)addr = frame.stack.getLong(0).i64;
>>
>> + frame.stack.pop(2);
>>
>> + break;
>>
>> }
>> ===================================================================
>>
>> --- interp_native_mips.cpp (revision 162)
>>
>> +++ interp_native_mips.cpp (working copy)
>> case JAVA_TYPE_DOUBLE:
>>
>> case JAVA_TYPE_LONG:
>>
>>
>> - args[argId+0] = prevFrame.stack.pick(pos-0).u;
>>
>> - args[argId+1] = prevFrame.stack.pick(pos-1).u;
>>
>> - argId += 2;
>>
>> - pos -= 2;
>>
>> - break;
>>
>>
>>
>> + Value2 val;
>>
>> + val.i64 = prevFrame.stack.getLong(pos-1).i64;
>>
>> + args[argId+0] = val.v[0].i;
>>
>> + args[argId+1] = val.v[1].i;
>>
>> + argId += 2;
>>
>> + pos -= 2;
>>
>> + break;
>>
>> I guess the fix is not fully right(does not handle double), but at least
>> the orignal patch do not have a consistent usage for "JAVA_TYPE_LONG".
>>
>> Tianwei
Re: MIPS Progress (WAS: The progress of MIPS patch work)
Posted by Alexey Varlamov <al...@gmail.com>.
2010/1/5 Tianwei <ti...@gmail.com>:
> Hi,Charles,
> Thanks. Since your patch does not include the makefile modification,
> Maybe I can send another patch which includes the makefile fixes based on
> your new patch. I think the most important requirements for the patch is
> that it should not break any existing things.
I believe the best way is to attach your new patch directly to the
relevant JIRA issue.
> BTW, can anyone point me to the right document for how to be involved in
> harmony development, i.e, checkin rules, licence issues, etc?
http://harmony.apache.org/get-involved.html
http://harmony.apache.org/contribution_policy.html
>
> Thanks.
>
> Tianwei
[skipped]
Re: MIPS Progress (WAS: The progress of MIPS patch work)
Posted by Tianwei <ti...@gmail.com>.
Hi,Charles,
Thanks. Since your patch does not include the makefile modification,
Maybe I can send another patch which includes the makefile fixes based on
your new patch. I think the most important requirements for the patch is
that it should not break any existing things.
BTW, can anyone point me to the right document for how to be involved in
harmony development, i.e, checkin rules, licence issues, etc?
Thanks.
Tianwei
On Tue, Jan 5, 2010 at 6:40 AM, Charles Hardin <ck...@gmail.com> wrote:
> Tianwei,
>
> Thanks - this is really good news. We should be able to return to
> looking at the MIPS work in a few weeks.
>
> I'll review the updates you sent and get a new patch submitted to
> HARMONY-6379 with the updates
>
> http://issues.apache.org/jira/browse/HARMONY-6379
>
> Charles
>
> On Sat, Jan 2, 2010 at 7:20 AM, Tianwei <ti...@gmail.com> wrote:
> > Hi, Charles,
> > I finally made a milestone, and get the interpreter works for
> > "HelloWorld", the following is the result:
> > stw@RAYS-b0f748fa:~/test$
> >
> /home/stw/harmony/harmony-nofetch/working_vm/build/linux_mips32_gcc_debug/deploy/jdk/jre/bin/java
> > -Xint HelloWorldApp
> > Hello World!
> >
> > Please ignore the previous mail, I made a mistake to comment out the
> > following line in
> > interp_native_mips.cpp:
> >
> > case JAVA_TYPE_DOUBLE:
> > case JAVA_TYPE_LONG:
> >
> > /* MIPS ABI requires 64 bit values to be
> > naturally aligned.
> > * pad here and increase array size if
> required.
> > */
> > if ((argId & 1) != 0) {
> > sz++;
> > arg_words2 = (uword*)ALLOC_FRAME((sz +
> 2)
> > * sizeof(uword));
> > if (!arg_words2) {
> > DIE(("no memory for frame"));
> > }
> > memcpy(arg_words2, arg_words, argId *
> > sizeof(uword));
> > FREE_FRAME(arg_words);
> > arg_words = arg_words2;
> > argId++;
> > }
> > Value2 val;
> > val.i64 = args[pos++].j;
> > arg_words[argId++] = val.v[0].i;
> > arg_words[argId++] = val.v[1].i;
> > break;
> >
> > I comment out the padding code, this will give me the following error,
> for
> > example:
> > JNIEXPORT void JNICALL
> > Java_java_util_zip_Inflater_setInputImpl (JNIEnv * env, jobject recv,
> > jbyteArray buf, jint off, jint
> > len,
> > jlong handle)
> > {
> > ............
> > }
> >
> > it has 6 arguments, in the interpreter, before call this JNI, if comment
> out
> > the padding code, the args is:
> > (gdb) p args[4]
> > $88 = 512
> > (gdb) p args[5]
> > $85 = 10515904
> > (gdb) p args[6]
> > $86 = 0
> >
> > args[5] and args[6] are for jlong, but the MIPS ABI need to align, so we
> > need to modify it as:
> > args[4] =512, args[5]=0
> > args[6] = 10515904, args[7]=0
> > when I enable the padding code, it works.
> >
> > Other issues are mainly about get and set value for JAVA_TYPE_LONG, I
> > believe the original patch has some problems, my fix looks like:
> > Value2 val;
> > val.i64 = args[pos++].j;
> > arg_words[argId++] = val.v[0].i;
> > arg_words[argId++] = val.v[1].i;
> > break;
> >
> >
> > summary:
> > (1) the interpreter works for "HelloWorld" based on Charles's patch on
> my
> > MIPS machines.
> > (2) I only knew very little about the harmony and JVM, only use gdb and
> > trace to solve the problem, I will continue to
> > learn them.
> > (3) next step, I will continue to test more cases and polish my code.
> >
> > BTW, Is there any updates for merging your patch into upstream? Since
> now
> > I make the interpreter work, I also can help testing.
> >
> > Thanks.
> >
> > Tianwei
> > On Tue, Dec 29, 2009 at 9:13 PM, Tianwei <ti...@gmail.com>
> wrote:
> >
> >> Well, after two day's debugging, the root cause is that there is some
> >> problem with the parameter passing in original patch on my machine, some
> of
> >> the code looks like:
> >> --- interpreter.cpp (revision 162)
> >>
> >> +++ interpreter.cpp (working copy)
> >>
> >> @@ -1691,10 +1691,10 @@
> >>
> >> case VM_DATA_TYPE_INT64:
> >>
> >> case VM_DATA_TYPE_F8:
> >>
> >> {
> >>
> >> - double *vaddr = (double*) addr;
> >>
> >> - *vaddr = frame.stack.getLong(0).d;
> >>
> >> - frame.stack.pop(2);
> >>
> >> - break;
> >>
> >> + //double *vaddr = (double*) addr;
> >>
> >> + *(I_64*)addr = frame.stack.getLong(0).i64;
> >>
> >> + frame.stack.pop(2);
> >>
> >> + break;
> >>
> >> }
> >> ===================================================================
> >>
> >> --- interp_native_mips.cpp (revision 162)
> >>
> >> +++ interp_native_mips.cpp (working copy)
> >> case JAVA_TYPE_DOUBLE:
> >>
> >> case JAVA_TYPE_LONG:
> >>
> >>
> >> - args[argId+0] = prevFrame.stack.pick(pos-0).u;
> >>
> >> - args[argId+1] = prevFrame.stack.pick(pos-1).u;
> >>
> >> - argId += 2;
> >>
> >> - pos -= 2;
> >>
> >> - break;
> >>
> >>
> >>
> >> + Value2 val;
> >>
> >> + val.i64 = prevFrame.stack.getLong(pos-1).i64;
> >>
> >> + args[argId+0] = val.v[0].i;
> >>
> >> + args[argId+1] = val.v[1].i;
> >>
> >> + argId += 2;
> >>
> >> + pos -= 2;
> >>
> >> + break;
> >>
> >> I guess the fix is not fully right(does not handle double), but at least
> >> the orignal patch do not have a consistent usage for "JAVA_TYPE_LONG".
> >>
> >> Tianwei
>
--
Sheng, Tianwei
Inst. of High Performance Computing
Dept. of Computer Sci. & Tech.
Tsinghua Univ.