You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by chunrong lai <ch...@gmail.com> on 2008/11/12 10:08:19 UTC
Enable the MACRO _DEBUG_CHECK_NULL_
hi, colleagues:
I am asking for the support of other committers to enable the MACRO
_DEBUG_CHECK_NULL_ (in include/jit_runtime_support.h), because of the rule
of M8 code frozen period.
I need the MACRO to enable the test case as in HARMONY-6013, which
throws uncaught NullPointerException. The exception comes with the stub of
vm_rt_monitor_enter. We had not met the reported exception because we
originally have two null checkings with that. The first one is just before
the stub. The second is inside the stub.
Now we find that the first null checking before the stub will be
removed after some aggressive optimizations. Also a commit r659128 moved the
null checking (and exception throwing) codes inside the stub into the
regions guarded by the disabled macro MACRO _DEBUG_CHECK_NULL_ . As the
result drlvm just fails in running such test cases.
Buqi is checking JIT codes to prepare another patch. But I am not sure
if we should wait and include the patch in M8. Or we can just try that after
code unfrozen.
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Tim Ellison <t....@gmail.com>.
I see the commit went in as r713673.
Are you done now, can we resume testing this build?
Regards,
Tim
chunrong lai wrote:
> Colleagues:
> Thanks a lot for your advices and discussions.
> I think that I got enough support to enable null-check inside the VM
> helper. I will commit it and upload a new snapshot soon.
>
> On Thu, Nov 13, 2008 at 3:05 PM, bu qi cheng <bu...@gmail.com> wrote:
>
>> Hi Mikhail
>>
>> Can you show us where is the magic for the monitorEnter? The eliminate
>> the chknull, the HLR magic is needed.
>>
>> Buqi
>>
>> On Thu, Nov 13, 2008 at 2:25 PM, Mikhail Fursov <mike.fursov@gmail.com
>>> wrote:
>>> AFAIR checknull is always precedes an operation that can cause exception
>> if
>>> one of its arguments is NULL. Since this operation may have side effects
>> -
>>> it can't be removed nor moved into try/cactch block by optimizer.
>>> In our case if the operation is a helper-call - it must fit this scheme:
>> do
>>> manual nullcheck.
>>>
>>> There should not be any performance problem here: if the helper is hot ->
>>> Jitrino.OPT will inline it using magics and eliminate the check.
>>> So I see no problems enabling null-check inside of the VM helper.
>>>
>>> --
>>> Mikhail Fursov
>>>
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by chunrong lai <ch...@gmail.com>.
Colleagues:
Thanks a lot for your advices and discussions.
I think that I got enough support to enable null-check inside the VM
helper. I will commit it and upload a new snapshot soon.
On Thu, Nov 13, 2008 at 3:05 PM, bu qi cheng <bu...@gmail.com> wrote:
> Hi Mikhail
>
> Can you show us where is the magic for the monitorEnter? The eliminate
> the chknull, the HLR magic is needed.
>
> Buqi
>
> On Thu, Nov 13, 2008 at 2:25 PM, Mikhail Fursov <mike.fursov@gmail.com
> >wrote:
>
> > AFAIR checknull is always precedes an operation that can cause exception
> if
> > one of its arguments is NULL. Since this operation may have side effects
> -
> > it can't be removed nor moved into try/cactch block by optimizer.
> > In our case if the operation is a helper-call - it must fit this scheme:
> do
> > manual nullcheck.
> >
> > There should not be any performance problem here: if the helper is hot ->
> > Jitrino.OPT will inline it using magics and eliminate the check.
> > So I see no problems enabling null-check inside of the VM helper.
> >
> > --
> > Mikhail Fursov
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Hi Mikhail
Can you show us where is the magic for the monitorEnter? The eliminate
the chknull, the HLR magic is needed.
Buqi
On Thu, Nov 13, 2008 at 2:25 PM, Mikhail Fursov <mi...@gmail.com>wrote:
> AFAIR checknull is always precedes an operation that can cause exception if
> one of its arguments is NULL. Since this operation may have side effects -
> it can't be removed nor moved into try/cactch block by optimizer.
> In our case if the operation is a helper-call - it must fit this scheme: do
> manual nullcheck.
>
> There should not be any performance problem here: if the helper is hot ->
> Jitrino.OPT will inline it using magics and eliminate the check.
> So I see no problems enabling null-check inside of the VM helper.
>
> --
> Mikhail Fursov
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Xiao-Feng Li <xi...@gmail.com>.
On Wed, Nov 19, 2008 at 3:20 AM, Mikhail Fursov <mi...@gmail.com> wrote:
> Hi Buqi,
>
> I think the issue with not-inlined monenter must be fixed - if monenter is
> "hot" it must be replaced with inlined magic code. Are you sure that
> monenter inst is "hot" in the code you use?
>
> The null check for monenter must be done in Java code at least once. If
> there is another nullcheck before the monenter -> the monenters one must be
> eliminated. If it isn'tt - it's a bug that must be fixed.
Agreed. We can't simply work around the issue by saying: hey I have
double-checks, it is safe anyway, although one check is enough.
Thanks,
xiaofeng
>
> On Tue, Nov 18, 2008 at 11:50 AM, bu qi cheng <bu...@gmail.com> wrote:
>
>> Hi Mikhail:
>>
>> Thanks for informatoin! I am sorry that I made a mistake in the
>> command line. The -Xem:client should be -Xem:server. I copied a wrong
>> command line. With your help, I found the magic code in
>> threadhelper.java:
>> @Inline
>> static void monitorExit(Object obj) {
>> Address lockWordPtr =
>> ObjectReference.fromObject(obj).toAddress().plus(LOCK_WORD_OFFSET);
>> int lockword = lockWordPtr.loadInt();
>> if (((lockword & (FAT_LOCK_MASK|RECURSION_MASK)))!=0) {
>> if ((lockword & FAT_LOCK_MASK)==0) {
>> lockword-=RECURSION_INC_IN_PLACE;
>> lockWordPtr.store(lockword);
>> return;
>> }
>> } else {
>> lockword&=~HI_BITS;
>> lockWordPtr.store(lockword);
>> return;
>> }
>> VMHelper.monitorExit(obj);
>> }
>>
>>
>> However, in the test case in last email, from the SD2 IR, we can not
>> find that the magic is called. So the same bug will happen. Of course
>> if add back the null pointer check in rth_get_lil_monitor_enter(), the
>> bug will disappear. However, the optimization chance will lost.
>>
>> Another problem is that in the inliner.cpp, in function
>> connectRegion() we can find code like:
>> Inst* tauNullCheckInst = tauNullChecked->getInst();
>> if(tauNullCheckInst->getOpcode() == Op_TauCheckNull) {
>> tauNullCheckInst->setDefArgModifier(NonNullThisArg);
>> }
>>
>> I think this means that if the inline done, the chknull for the
>> argument will be un-optimizable. If the magic is called, no error, and
>> no optimization at the same time. Am I right?
>>
>> Thanks!
>>
>> Buqi
>>
>>
>>
>> Thanks!
>>
>> Buqi
>>
>
>
>
> --
> Mikhail Fursov
>
--
http://xiao-feng.blogspot.com
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Hi Mikhail:
You are right. The test case I wrote is not right, in which
monenter will only be called once although the loop run a lot of
times. After fixed the test case, I can see the inlining of the magic
code.
Also, there will be no chknull in the translated
"monitorEnterUseReservation" code, although there is DefArg of the
object. So, there problem I mentioned in the inline.cpp will not
exsit.
Now, I agree with you on the roll back of code in VM helper for
monitorEnter(in jit_lock_rt_support...), and I will close the
unnecessory raised issue. Thanks a lot for the helping on the problem.
Thanks!
Buqi
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Mikhail Fursov <mi...@gmail.com>.
Hi Buqi,
I think the issue with not-inlined monenter must be fixed - if monenter is
"hot" it must be replaced with inlined magic code. Are you sure that
monenter inst is "hot" in the code you use?
The null check for monenter must be done in Java code at least once. If
there is another nullcheck before the monenter -> the monenters one must be
eliminated. If it isn'tt - it's a bug that must be fixed.
On Tue, Nov 18, 2008 at 11:50 AM, bu qi cheng <bu...@gmail.com> wrote:
> Hi Mikhail:
>
> Thanks for informatoin! I am sorry that I made a mistake in the
> command line. The -Xem:client should be -Xem:server. I copied a wrong
> command line. With your help, I found the magic code in
> threadhelper.java:
> @Inline
> static void monitorExit(Object obj) {
> Address lockWordPtr =
> ObjectReference.fromObject(obj).toAddress().plus(LOCK_WORD_OFFSET);
> int lockword = lockWordPtr.loadInt();
> if (((lockword & (FAT_LOCK_MASK|RECURSION_MASK)))!=0) {
> if ((lockword & FAT_LOCK_MASK)==0) {
> lockword-=RECURSION_INC_IN_PLACE;
> lockWordPtr.store(lockword);
> return;
> }
> } else {
> lockword&=~HI_BITS;
> lockWordPtr.store(lockword);
> return;
> }
> VMHelper.monitorExit(obj);
> }
>
>
> However, in the test case in last email, from the SD2 IR, we can not
> find that the magic is called. So the same bug will happen. Of course
> if add back the null pointer check in rth_get_lil_monitor_enter(), the
> bug will disappear. However, the optimization chance will lost.
>
> Another problem is that in the inliner.cpp, in function
> connectRegion() we can find code like:
> Inst* tauNullCheckInst = tauNullChecked->getInst();
> if(tauNullCheckInst->getOpcode() == Op_TauCheckNull) {
> tauNullCheckInst->setDefArgModifier(NonNullThisArg);
> }
>
> I think this means that if the inline done, the chknull for the
> argument will be un-optimizable. If the magic is called, no error, and
> no optimization at the same time. Am I right?
>
> Thanks!
>
> Buqi
>
>
>
> Thanks!
>
> Buqi
>
--
Mikhail Fursov
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Hi Mikhail:
Thanks for informatoin! I am sorry that I made a mistake in the
command line. The -Xem:client should be -Xem:server. I copied a wrong
command line. With your help, I found the magic code in
threadhelper.java:
@Inline
static void monitorExit(Object obj) {
Address lockWordPtr =
ObjectReference.fromObject(obj).toAddress().plus(LOCK_WORD_OFFSET);
int lockword = lockWordPtr.loadInt();
if (((lockword & (FAT_LOCK_MASK|RECURSION_MASK)))!=0) {
if ((lockword & FAT_LOCK_MASK)==0) {
lockword-=RECURSION_INC_IN_PLACE;
lockWordPtr.store(lockword);
return;
}
} else {
lockword&=~HI_BITS;
lockWordPtr.store(lockword);
return;
}
VMHelper.monitorExit(obj);
}
However, in the test case in last email, from the SD2 IR, we can not
find that the magic is called. So the same bug will happen. Of course
if add back the null pointer check in rth_get_lil_monitor_enter(), the
bug will disappear. However, the optimization chance will lost.
Another problem is that in the inliner.cpp, in function
connectRegion() we can find code like:
Inst* tauNullCheckInst = tauNullChecked->getInst();
if(tauNullCheckInst->getOpcode() == Op_TauCheckNull) {
tauNullCheckInst->setDefArgModifier(NonNullThisArg);
}
I think this means that if the inline done, the chknull for the
argument will be un-optimizable. If the magic is called, no error, and
no optimization at the same time. Am I right?
Thanks!
Buqi
Thanks!
Buqi
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Mikhail Fursov <mi...@gmail.com>.
Buqi,
Please check the
"helper_inliner.cpp"<http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/jitrino/src/optimizer/helper_inliner.cpp?view=log>file.
+ AFAIR helper inlining is enabled only in 'server' mode.
--
Mikhail Fursov
On Mon, Nov 17, 2008 at 9:21 AM, bu qi cheng <bu...@gmail.com> wrote:
> Hi Mikhail:
>
> I can not find there is magic for the monitorEnter monitorExit. A test
> case is done like following:
>
> public class Btest5710 {
>
> Object obj;
> int j = 0;
> public static void main(String [] args) {
> (new Btest5710()).test();
> }
> public void test() {
> int i;
> System.err.println("Start Btest5710 test...");
> try {
> for (i = 0; i < 10000000; i++) {
> sync();
> }
> System.err.println("FAILED: NullPointerException was expected");
> System.exit(-99);
> } catch (NullPointerException e) {
> System.err.println("PASSED: " + e);
> } catch (Throwable e) {
> System.err.println("Unexpected exception was thrown:");
> e.printStackTrace();
> System.err.println("FAILED");
> System.exit(-99);
> }
> }
> void sync() {
> int i;
> //chknull
> for (i = 0; i < 10; i++) {
> j++;
> if (j == 90000000) {
> synchronized (obj) {
> obj = new Object();
> }
> }
> }
> }
> }
>
> Command is like following:
>
> $
> ../harmony/working_vm/build/windows_x86_msvc_debug/deploy/jdk/jre/bin/java
> -Xem:client -XX:jit.p.filter=Btest5710. -XX:jit.p.arg.log=ct,irdump,info
> Btest5710
> Same bug will happen and we see no magic code is generated. SD2 code for
> sync() is generated also, and we can see that the chknull is still not
> work,
> same as the monenter.
>
>
> Regards!
>
> Buqi
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Hi Mikhail:
I can not find there is magic for the monitorEnter monitorExit. A test
case is done like following:
public class Btest5710 {
Object obj;
int j = 0;
public static void main(String [] args) {
(new Btest5710()).test();
}
public void test() {
int i;
System.err.println("Start Btest5710 test...");
try {
for (i = 0; i < 10000000; i++) {
sync();
}
System.err.println("FAILED: NullPointerException was expected");
System.exit(-99);
} catch (NullPointerException e) {
System.err.println("PASSED: " + e);
} catch (Throwable e) {
System.err.println("Unexpected exception was thrown:");
e.printStackTrace();
System.err.println("FAILED");
System.exit(-99);
}
}
void sync() {
int i;
//chknull
for (i = 0; i < 10; i++) {
j++;
if (j == 90000000) {
synchronized (obj) {
obj = new Object();
}
}
}
}
}
Command is like following:
$ ../harmony/working_vm/build/windows_x86_msvc_debug/deploy/jdk/jre/bin/java
-Xem:client -XX:jit.p.filter=Btest5710. -XX:jit.p.arg.log=ct,irdump,info
Btest5710
Same bug will happen and we see no magic code is generated. SD2 code for
sync() is generated also, and we can see that the chknull is still not work,
same as the monenter.
Regards!
Buqi
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Mikhail Fursov <mi...@gmail.com>.
AFAIR checknull is always precedes an operation that can cause exception if
one of its arguments is NULL. Since this operation may have side effects -
it can't be removed nor moved into try/cactch block by optimizer.
In our case if the operation is a helper-call - it must fit this scheme: do
manual nullcheck.
There should not be any performance problem here: if the helper is hot ->
Jitrino.OPT will inline it using magics and eliminate the check.
So I see no problems enabling null-check inside of the VM helper.
--
Mikhail Fursov
On Wed, Nov 12, 2008 at 10:03 PM, chunrong lai <ch...@gmail.com>wrote:
> Ah. Thanks for the reply.
> So you propose to change the rule (2) of MIkhail from
> (2) JIT in LIR can replace checknull with hardware exception check only if
> 'checknull' op is not in a try-catch block.
> to
> (2)* JIT in LIR can replace checknull with hardware exception check only
> if
> 'checknull' op is not in a try-catch block, include the try-catch block of
> any level of caller (require unwinding check).
>
> With the enhancement I see that the chknull will not be removed/replaced as
> observed.
> Please correct me if I misunderstand you.
> If so Mihail's judgement of "void sync() is not inlined" is irrelevant
> because we always need to do the unwinding check.
> Mikhail, what is your advice?
>
> Anyway this modification looks not a minor change to me.
> On Wed, Nov 12, 2008 at 11:34 PM, bu qi cheng <bu...@gmail.com>
> wrote:
>
> > On Wed, Nov 12, 2008 at 10:48 PM, chunrong lai <chunronglai@gmail.com
> > >wrote:
> >
> > > hi, Buqi:
> > > I am not very convinced to add some MonitorEnter specific code in
> > > IRBuilder Code. Why only MonitorEnter is impacted here?
> >
> >
> > Currently, we only know that moniorEnter and monitorExit have no null
> > pointer check code.
> > An assumption is that if MonitorExit is excuted, there will alway be a
> > MonitorEnter before it. I think java verfier will do this checking.
> Please
> > double check if it's right.
> >
> > >
> > >
> > > We can see that the NPE is from the removal of two checknull
> > operations.
> > > If we target to the inproper removal of the first checknull from
> JIT.
> > > The modification or the optimization rule should be MonitorEnter
> > > independent. So I do not see MonitorEnter related issue in Mikhail's
> > > description.
> >
> >
> > It seems for monitorEnter/monitorExit, we can not do in this way. Because
> > hardware segment fault can not replace the null pointer check. Especially
> > for the test case.
> > Currently, the implementation of HARMONY is that if the check is not
> > catched
> > by any code of current, or caller(after inline), it will be eliminated. I
> > don't think it's right for all situation.
> >
> >
> > >
> > > If we target to the removal of the second checknull in the
> > stub/helper.
> > > The modification is MonitorEnter dependent. It is clear that the
> removal
> > is
> > > due to r659128 who changed the helpers. (It is a little strange to only
> > > change the IRBuilder::genMonitorEnter, with the knowledge of no null
> > > checking code in the stub/helpers).
> >
> >
> > r659128 exposed more optimization chance for the JIT.
> > HARMONY did other work to make sure that the check is right. Such as, if
> we
> > pass a NULL object by arguement, HARMONY will make the chknull always
> work.
> > It's just like what we are doing.
> >
> >
> > >
> > > I myself am a little confused about the hardware exception check.
> What
> > is
> > > the background, expected behavior and triggering condition? (Mikhail
> only
> > > mentioned that the optimization happens only if some conditions, does
> it
> > > always heppen if so?) Maybe we can just give up the hardware exception
> > > check.
> >
> >
> > I think hardware exception only work when there is no catcher in all
> > callers. Or the caller can catch all exceptions. So, when doing so, I
> think
> > the unwind checking for exception handler tables in all callers' is
> needed.
> >
> >
> > >
> > > On Wed, Nov 12, 2008 at 10:20 PM, bu qi cheng <bu...@gmail.com>
> > > wrote:
> > >
> > > > Hi Mikhail:
> > > >
> > > > One question is that if Harmony did NULL Pointer Check
> Elimination?
> > I
> > > > am not sure if 1) is done.
> > > > For this problem caused by 2). A patch is like following:
> > > >
> > > >
> > > >
> > > > IRBuilder.cpp
> > > >
> > > > void
> > > >
> > > > IRBuilder::genMonitorEnter(Opnd* src) {
> > > >
> > > > src = propagateCopy(src);
> > > >
> > > > Opnd *tauNullChecked = genTauCheckNull(src);
> > > >
> > > > * Inst* inst = tauNullChecked->getInst(); //Added*
> > > >
> > > > * if (inst->getOpcode() == Op_TauCheckNull) //Added*
> > > >
> > > > * inst->setDefArgModifier(NonNullThisArg); //Added*
> > > >
> > > > appendInst(instFactory->makeTauMonitorEnter(src, tauNullChecked));
> > > >
> > > > }
> > > >
> > > > By set the modifier of TauCheckNull instruction before the
> MonitorEnter
> > > > instruction. If the "chknull" is not optimized by 1). HARMONY will
> > always
> > > > generate the LIR check code for it.
> > > > With this way, there is no need to generate two version helper code
> for
> > > > monitorEnter.
> > > >
> > > > Please double check if it's right.
> > > >
> > > >
> > > > Thanks!
> > > >
> > > > Buqi
> > > >
> > >
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by chunrong lai <ch...@gmail.com>.
Ah. Thanks for the reply.
So you propose to change the rule (2) of MIkhail from
(2) JIT in LIR can replace checknull with hardware exception check only if
'checknull' op is not in a try-catch block.
to
(2)* JIT in LIR can replace checknull with hardware exception check only if
'checknull' op is not in a try-catch block, include the try-catch block of
any level of caller (require unwinding check).
With the enhancement I see that the chknull will not be removed/replaced as
observed.
Please correct me if I misunderstand you.
If so Mihail's judgement of "void sync() is not inlined" is irrelevant
because we always need to do the unwinding check.
Mikhail, what is your advice?
Anyway this modification looks not a minor change to me.
On Wed, Nov 12, 2008 at 11:34 PM, bu qi cheng <bu...@gmail.com> wrote:
> On Wed, Nov 12, 2008 at 10:48 PM, chunrong lai <chunronglai@gmail.com
> >wrote:
>
> > hi, Buqi:
> > I am not very convinced to add some MonitorEnter specific code in
> > IRBuilder Code. Why only MonitorEnter is impacted here?
>
>
> Currently, we only know that moniorEnter and monitorExit have no null
> pointer check code.
> An assumption is that if MonitorExit is excuted, there will alway be a
> MonitorEnter before it. I think java verfier will do this checking. Please
> double check if it's right.
>
> >
> >
> > We can see that the NPE is from the removal of two checknull
> operations.
> > If we target to the inproper removal of the first checknull from JIT.
> > The modification or the optimization rule should be MonitorEnter
> > independent. So I do not see MonitorEnter related issue in Mikhail's
> > description.
>
>
> It seems for monitorEnter/monitorExit, we can not do in this way. Because
> hardware segment fault can not replace the null pointer check. Especially
> for the test case.
> Currently, the implementation of HARMONY is that if the check is not
> catched
> by any code of current, or caller(after inline), it will be eliminated. I
> don't think it's right for all situation.
>
>
> >
> > If we target to the removal of the second checknull in the
> stub/helper.
> > The modification is MonitorEnter dependent. It is clear that the removal
> is
> > due to r659128 who changed the helpers. (It is a little strange to only
> > change the IRBuilder::genMonitorEnter, with the knowledge of no null
> > checking code in the stub/helpers).
>
>
> r659128 exposed more optimization chance for the JIT.
> HARMONY did other work to make sure that the check is right. Such as, if we
> pass a NULL object by arguement, HARMONY will make the chknull always work.
> It's just like what we are doing.
>
>
> >
> > I myself am a little confused about the hardware exception check. What
> is
> > the background, expected behavior and triggering condition? (Mikhail only
> > mentioned that the optimization happens only if some conditions, does it
> > always heppen if so?) Maybe we can just give up the hardware exception
> > check.
>
>
> I think hardware exception only work when there is no catcher in all
> callers. Or the caller can catch all exceptions. So, when doing so, I think
> the unwind checking for exception handler tables in all callers' is needed.
>
>
> >
> > On Wed, Nov 12, 2008 at 10:20 PM, bu qi cheng <bu...@gmail.com>
> > wrote:
> >
> > > Hi Mikhail:
> > >
> > > One question is that if Harmony did NULL Pointer Check Elimination?
> I
> > > am not sure if 1) is done.
> > > For this problem caused by 2). A patch is like following:
> > >
> > >
> > >
> > > IRBuilder.cpp
> > >
> > > void
> > >
> > > IRBuilder::genMonitorEnter(Opnd* src) {
> > >
> > > src = propagateCopy(src);
> > >
> > > Opnd *tauNullChecked = genTauCheckNull(src);
> > >
> > > * Inst* inst = tauNullChecked->getInst(); //Added*
> > >
> > > * if (inst->getOpcode() == Op_TauCheckNull) //Added*
> > >
> > > * inst->setDefArgModifier(NonNullThisArg); //Added*
> > >
> > > appendInst(instFactory->makeTauMonitorEnter(src, tauNullChecked));
> > >
> > > }
> > >
> > > By set the modifier of TauCheckNull instruction before the MonitorEnter
> > > instruction. If the "chknull" is not optimized by 1). HARMONY will
> always
> > > generate the LIR check code for it.
> > > With this way, there is no need to generate two version helper code for
> > > monitorEnter.
> > >
> > > Please double check if it's right.
> > >
> > >
> > > Thanks!
> > >
> > > Buqi
> > >
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
On Wed, Nov 12, 2008 at 10:48 PM, chunrong lai <ch...@gmail.com>wrote:
> hi, Buqi:
> I am not very convinced to add some MonitorEnter specific code in
> IRBuilder Code. Why only MonitorEnter is impacted here?
Currently, we only know that moniorEnter and monitorExit have no null
pointer check code.
An assumption is that if MonitorExit is excuted, there will alway be a
MonitorEnter before it. I think java verfier will do this checking. Please
double check if it's right.
>
>
> We can see that the NPE is from the removal of two checknull operations.
> If we target to the inproper removal of the first checknull from JIT.
> The modification or the optimization rule should be MonitorEnter
> independent. So I do not see MonitorEnter related issue in Mikhail's
> description.
It seems for monitorEnter/monitorExit, we can not do in this way. Because
hardware segment fault can not replace the null pointer check. Especially
for the test case.
Currently, the implementation of HARMONY is that if the check is not catched
by any code of current, or caller(after inline), it will be eliminated. I
don't think it's right for all situation.
>
> If we target to the removal of the second checknull in the stub/helper.
> The modification is MonitorEnter dependent. It is clear that the removal is
> due to r659128 who changed the helpers. (It is a little strange to only
> change the IRBuilder::genMonitorEnter, with the knowledge of no null
> checking code in the stub/helpers).
r659128 exposed more optimization chance for the JIT.
HARMONY did other work to make sure that the check is right. Such as, if we
pass a NULL object by arguement, HARMONY will make the chknull always work.
It's just like what we are doing.
>
> I myself am a little confused about the hardware exception check. What is
> the background, expected behavior and triggering condition? (Mikhail only
> mentioned that the optimization happens only if some conditions, does it
> always heppen if so?) Maybe we can just give up the hardware exception
> check.
I think hardware exception only work when there is no catcher in all
callers. Or the caller can catch all exceptions. So, when doing so, I think
the unwind checking for exception handler tables in all callers' is needed.
>
> On Wed, Nov 12, 2008 at 10:20 PM, bu qi cheng <bu...@gmail.com>
> wrote:
>
> > Hi Mikhail:
> >
> > One question is that if Harmony did NULL Pointer Check Elimination? I
> > am not sure if 1) is done.
> > For this problem caused by 2). A patch is like following:
> >
> >
> >
> > IRBuilder.cpp
> >
> > void
> >
> > IRBuilder::genMonitorEnter(Opnd* src) {
> >
> > src = propagateCopy(src);
> >
> > Opnd *tauNullChecked = genTauCheckNull(src);
> >
> > * Inst* inst = tauNullChecked->getInst(); //Added*
> >
> > * if (inst->getOpcode() == Op_TauCheckNull) //Added*
> >
> > * inst->setDefArgModifier(NonNullThisArg); //Added*
> >
> > appendInst(instFactory->makeTauMonitorEnter(src, tauNullChecked));
> >
> > }
> >
> > By set the modifier of TauCheckNull instruction before the MonitorEnter
> > instruction. If the "chknull" is not optimized by 1). HARMONY will always
> > generate the LIR check code for it.
> > With this way, there is no need to generate two version helper code for
> > monitorEnter.
> >
> > Please double check if it's right.
> >
> >
> > Thanks!
> >
> > Buqi
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by chunrong lai <ch...@gmail.com>.
hi, Buqi:
I am not very convinced to add some MonitorEnter specific code in
IRBuilder Code. Why only MonitorEnter is impacted here?
We can see that the NPE is from the removal of two checknull operations.
If we target to the inproper removal of the first checknull from JIT.
The modification or the optimization rule should be MonitorEnter
independent. So I do not see MonitorEnter related issue in Mikhail's
description.
If we target to the removal of the second checknull in the stub/helper.
The modification is MonitorEnter dependent. It is clear that the removal is
due to r659128 who changed the helpers. (It is a little strange to only
change the IRBuilder::genMonitorEnter, with the knowledge of no null
checking code in the stub/helpers).
I myself am a little confused about the hardware exception check. What is
the background, expected behavior and triggering condition? (Mikhail only
mentioned that the optimization happens only if some conditions, does it
always heppen if so?) Maybe we can just give up the hardware exception
check.
On Wed, Nov 12, 2008 at 10:20 PM, bu qi cheng <bu...@gmail.com> wrote:
> Hi Mikhail:
>
> One question is that if Harmony did NULL Pointer Check Elimination? I
> am not sure if 1) is done.
> For this problem caused by 2). A patch is like following:
>
>
>
> IRBuilder.cpp
>
> void
>
> IRBuilder::genMonitorEnter(Opnd* src) {
>
> src = propagateCopy(src);
>
> Opnd *tauNullChecked = genTauCheckNull(src);
>
> * Inst* inst = tauNullChecked->getInst(); //Added*
>
> * if (inst->getOpcode() == Op_TauCheckNull) //Added*
>
> * inst->setDefArgModifier(NonNullThisArg); //Added*
>
> appendInst(instFactory->makeTauMonitorEnter(src, tauNullChecked));
>
> }
>
> By set the modifier of TauCheckNull instruction before the MonitorEnter
> instruction. If the "chknull" is not optimized by 1). HARMONY will always
> generate the LIR check code for it.
> With this way, there is no need to generate two version helper code for
> monitorEnter.
>
> Please double check if it's right.
>
>
> Thanks!
>
> Buqi
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Hi Mikhail:
One question is that if Harmony did NULL Pointer Check Elimination? I
am not sure if 1) is done.
For this problem caused by 2). A patch is like following:
IRBuilder.cpp
void
IRBuilder::genMonitorEnter(Opnd* src) {
src = propagateCopy(src);
Opnd *tauNullChecked = genTauCheckNull(src);
* Inst* inst = tauNullChecked->getInst(); //Added*
* if (inst->getOpcode() == Op_TauCheckNull) //Added*
* inst->setDefArgModifier(NonNullThisArg); //Added*
appendInst(instFactory->makeTauMonitorEnter(src, tauNullChecked));
}
By set the modifier of TauCheckNull instruction before the MonitorEnter
instruction. If the "chknull" is not optimized by 1). HARMONY will always
generate the LIR check code for it.
With this way, there is no need to generate two version helper code for
monitorEnter.
Please double check if it's right.
Thanks!
Buqi
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by bu qi cheng <bu...@gmail.com>.
Currently, there are two kind modifier for TauCheckNull: Exception and
DefArg.
Inst* InstFactory::makeTauCheckNull(Opnd* dst, Opnd* base) {
assert(dst->getType()->tag == Type::Tau);
return makeInst(Op_TauCheckNull, *
Modifier(Exception_Sometimes)|Modifier(DefArgNoModifier),*
Type::Tau, dst, base);
}
Maybe, it's not suitable to use Modifer "DefArg" (NonNullThisArg) for
chknull instruction (Although it's right curerntly).
I think SrcNonNullModifier is more suitable to be used to solve the problem.
Any commnets?
Thanks!
Buqi
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by chunrong lai <ch...@gmail.com>.
hi, Mikhail:
Thanks for the reply.
I'd like to delay the MACRO enabling because it is too difficult for me
to maintain and choose from two version of RT helpers.
In my understanding the problem of HARMONY-6013 comes from (2), the
checknull is replaced with the hardware exception check (does the
optimization always happen if 'checknull' op is not in a try-catch block).
Unfortunately the hardware exception check is inprecise or incompatible with
the Java exception check/catch in this time. It just call the stub/helper
directly and the helper just crashed if without NPE checking code inside.
Does it make sense?
On Wed, Nov 12, 2008 at 7:16 PM, Mikhail Fursov <mi...@gmail.com>wrote:
> Chunrong,
> Here are simple rules for JIT for checknull operation:
> 1) JIT in HIR can delete checknull only if it can prove (has tauSafe
> operand) that another check for the same SSA opnd was performed in
> dominated
> code.
> 2) JIT in LIR can replace checknull with hardware exception check only if
> 'checknull' op is not in a try-catch block.
>
> For the method in HARMONY-6013 JIT can eliminate checknull before the
> monenter helper call only if "void sync()" is not inlined. If it's inlined
> the monenter instruction is placed into the try-catch block of the parent
> method.
> It means that monenter helper that is called from JIT must have a check for
> NULLs.
> From the performance POV it could be worth to having two kinds of monenter
> helpers: one with a check for null and another without.
>
> Hope it helps.
> MIkhail
>
>
> On Wed, Nov 12, 2008 at 3:53 PM, chunrong lai <ch...@gmail.com>
> wrote:
>
> > Thanks.
> > Mikhail, can you kindly help to advice on this?
> >
> > On Wed, Nov 12, 2008 at 5:24 PM, Alexei Fedotov <
> alexei.fedotov@gmail.com
> > >wrote:
> >
> > > I wonder if may get Mikhail advice on this.
> > >
> > > On Wed, Nov 12, 2008 at 12:08 PM, chunrong lai <ch...@gmail.com>
> > > wrote:
> > > > hi, colleagues:
> > > > I am asking for the support of other committers to enable the
> MACRO
> > > > _DEBUG_CHECK_NULL_ (in include/jit_runtime_support.h), because of the
> > > rule
> > > > of M8 code frozen period.
> > > > I need the MACRO to enable the test case as in HARMONY-6013,
> which
> > > > throws uncaught NullPointerException. The exception comes with the
> stub
> > > of
> > > > vm_rt_monitor_enter. We had not met the reported exception because
> we
> > > > originally have two null checkings with that. The first one is just
> > > before
> > > > the stub. The second is inside the stub.
> > > > Now we find that the first null checking before the stub will be
> > > > removed after some aggressive optimizations. Also a commit r659128
> > moved
> > > the
> > > > null checking (and exception throwing) codes inside the stub into the
> > > > regions guarded by the disabled macro MACRO _DEBUG_CHECK_NULL_ . As
> > the
> > > > result drlvm just fails in running such test cases.
> > > > Buqi is checking JIT codes to prepare another patch. But I am not
> > > sure
> > > > if we should wait and include the patch in M8. Or we can just try
> that
> > > after
> > > > code unfrozen.
> > > >
> > >
> > >
> > >
> > > --
> > > С уважением,
> > > Алексей Федотов,
> > > Телеком Экспресс
> > >
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Mikhail Fursov <mi...@gmail.com>.
Chunrong,
Here are simple rules for JIT for checknull operation:
1) JIT in HIR can delete checknull only if it can prove (has tauSafe
operand) that another check for the same SSA opnd was performed in dominated
code.
2) JIT in LIR can replace checknull with hardware exception check only if
'checknull' op is not in a try-catch block.
For the method in HARMONY-6013 JIT can eliminate checknull before the
monenter helper call only if "void sync()" is not inlined. If it's inlined
the monenter instruction is placed into the try-catch block of the parent
method.
It means that monenter helper that is called from JIT must have a check for
NULLs.
From the performance POV it could be worth to having two kinds of monenter
helpers: one with a check for null and another without.
Hope it helps.
MIkhail
On Wed, Nov 12, 2008 at 3:53 PM, chunrong lai <ch...@gmail.com> wrote:
> Thanks.
> Mikhail, can you kindly help to advice on this?
>
> On Wed, Nov 12, 2008 at 5:24 PM, Alexei Fedotov <alexei.fedotov@gmail.com
> >wrote:
>
> > I wonder if may get Mikhail advice on this.
> >
> > On Wed, Nov 12, 2008 at 12:08 PM, chunrong lai <ch...@gmail.com>
> > wrote:
> > > hi, colleagues:
> > > I am asking for the support of other committers to enable the MACRO
> > > _DEBUG_CHECK_NULL_ (in include/jit_runtime_support.h), because of the
> > rule
> > > of M8 code frozen period.
> > > I need the MACRO to enable the test case as in HARMONY-6013, which
> > > throws uncaught NullPointerException. The exception comes with the stub
> > of
> > > vm_rt_monitor_enter. We had not met the reported exception because we
> > > originally have two null checkings with that. The first one is just
> > before
> > > the stub. The second is inside the stub.
> > > Now we find that the first null checking before the stub will be
> > > removed after some aggressive optimizations. Also a commit r659128
> moved
> > the
> > > null checking (and exception throwing) codes inside the stub into the
> > > regions guarded by the disabled macro MACRO _DEBUG_CHECK_NULL_ . As
> the
> > > result drlvm just fails in running such test cases.
> > > Buqi is checking JIT codes to prepare another patch. But I am not
> > sure
> > > if we should wait and include the patch in M8. Or we can just try that
> > after
> > > code unfrozen.
> > >
> >
> >
> >
> > --
> > С уважением,
> > Алексей Федотов,
> > Телеком Экспресс
> >
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by chunrong lai <ch...@gmail.com>.
Thanks.
Mikhail, can you kindly help to advice on this?
On Wed, Nov 12, 2008 at 5:24 PM, Alexei Fedotov <al...@gmail.com>wrote:
> I wonder if may get Mikhail advice on this.
>
> On Wed, Nov 12, 2008 at 12:08 PM, chunrong lai <ch...@gmail.com>
> wrote:
> > hi, colleagues:
> > I am asking for the support of other committers to enable the MACRO
> > _DEBUG_CHECK_NULL_ (in include/jit_runtime_support.h), because of the
> rule
> > of M8 code frozen period.
> > I need the MACRO to enable the test case as in HARMONY-6013, which
> > throws uncaught NullPointerException. The exception comes with the stub
> of
> > vm_rt_monitor_enter. We had not met the reported exception because we
> > originally have two null checkings with that. The first one is just
> before
> > the stub. The second is inside the stub.
> > Now we find that the first null checking before the stub will be
> > removed after some aggressive optimizations. Also a commit r659128 moved
> the
> > null checking (and exception throwing) codes inside the stub into the
> > regions guarded by the disabled macro MACRO _DEBUG_CHECK_NULL_ . As the
> > result drlvm just fails in running such test cases.
> > Buqi is checking JIT codes to prepare another patch. But I am not
> sure
> > if we should wait and include the patch in M8. Or we can just try that
> after
> > code unfrozen.
> >
>
>
>
> --
> С уважением,
> Алексей Федотов,
> Телеком Экспресс
>
Re: Enable the MACRO _DEBUG_CHECK_NULL_
Posted by Alexei Fedotov <al...@gmail.com>.
I wonder if may get Mikhail advice on this.
On Wed, Nov 12, 2008 at 12:08 PM, chunrong lai <ch...@gmail.com> wrote:
> hi, colleagues:
> I am asking for the support of other committers to enable the MACRO
> _DEBUG_CHECK_NULL_ (in include/jit_runtime_support.h), because of the rule
> of M8 code frozen period.
> I need the MACRO to enable the test case as in HARMONY-6013, which
> throws uncaught NullPointerException. The exception comes with the stub of
> vm_rt_monitor_enter. We had not met the reported exception because we
> originally have two null checkings with that. The first one is just before
> the stub. The second is inside the stub.
> Now we find that the first null checking before the stub will be
> removed after some aggressive optimizations. Also a commit r659128 moved the
> null checking (and exception throwing) codes inside the stub into the
> regions guarded by the disabled macro MACRO _DEBUG_CHECK_NULL_ . As the
> result drlvm just fails in running such test cases.
> Buqi is checking JIT codes to prepare another patch. But I am not sure
> if we should wait and include the patch in M8. Or we can just try that after
> code unfrozen.
>
--
С уважением,
Алексей Федотов,
Телеком Экспресс