backedge checks

[Andrew Haley]

> Hi folks,
> 
> In bytecodeInterpreter.c is this wonderful macro
> 
> #define DO_BACKEDGE_CHECKS(skip, branch_pc)\
>     if ((skip) <= 0) {\
>       if (UseCompiler && UseLoopCounter) {\
>         bool do_OSR = UseOnStackReplacement;\
>         BACKEDGE_COUNT->increment();\
>         if (do_OSR) do_OSR = BACKEDGE_COUNT->reached_InvocationLimit();\
>         if (do_OSR) {\
>           nmethod*  osr_nmethod;\
>           OSR_REQUEST(osr_nmethod, branch_pc);\
>           if (osr_nmethod != NULL && osr_nmethod->osr_entry_bci() !=InvalidOSREntryBci) {          \
>             intptr_t* buf;\
>             CALL_VM(buf=SharedRuntime::OSR_migration_begin(THREAD), handle_exception);              \
>             istate->set_msg(do_osr);\
>             istate->set_osr_buf((address)buf);\
>             istate->set_osr_entry(osr_nmethod->osr_entry());\
>             return;\
>           }\
>         } else {\
>           INCR_INVOCATION_COUNT;\
>           SAFEPOINT;\
>         }\
>       }  /* UseCompiler ... */\
>       INCR_INVOCATION_COUNT;\
>       SAFEPOINT;\
>     }
> 
> 
> This macro is invoked in every branch (although the body is only
> executed on backwards branches).
> 
> Firstly, is it just me, or does it do INCR_INVOCATION_COUNT; SAFEPOINT
> twice on every backwards branch. Surely a mistake?

It looks like that to me.

> Secondly, can someone tell me under what circumstances in zero
> UseCompiler would ever be true (and no, it doesn't resolve to a
> constant.

This stuff is AFAIK used to decide when to compile a method with Shark.

> Thirdly, it should be possible to avoid the SAFEPOINT checks. SAFEPOINT
> does...
> 
> #define SAFEPOINT \
>     if ( SafepointSynchronize::is_synchronizing()) { \
>         { \
>           /* zap freed handles rather than GC'ing them */ \
>           HandleMarkCleaner __hmc(THREAD); \
>         } \
>         CALL_VM(SafepointSynchronize::block(THREAD), handle_exception); \
>     }
> 
> 
> However, there is an upcall notice_safepoints() which safepoint.cpp
> calls whenever it is setting is_synchronizing(). This upcall is there to
> notify the interpreter that it needs to run to a GC safepoint. A
> corresponding call ignore_safepoints() is called when the interpreter
> can safely ignore safepoints. So there should be no need for the
> interpreter to continually check for safepoints.
> 
> notice_safepoints() and ignore_safepoints() in the template interpreter
> do indeed do something sensible. However, in the bytecode Interpreter
> the implementation is just {}
> 
> The way it would work is...
> 
> When compiling gcc bytecodeInterpreter.cpp uses a dispatch table rather
> than a switch statement. Currently this is defined as
> 
>   const static void* const opclabels_data[256] = {
> /* 0x00 */ &&opc_nop,
> &&opc_aconst_null,&&opc_iconst_m1,&&opc_iconst_0,
> ...
>       };
>   register uintptr_t *dispatch_table = (uintptr_t*)&opclabels_data[0];
> 
> We would change this to
> 
>   const static void* opclabels_data[256] = {
> /* 0x00 */ &&opc_nop,
> &&opc_aconst_null,&&opc_iconst_m1,&&opc_iconst_0,
> ...
>       };
> 
> IE. I have just removed the 'const' because we are going to change this
> dynamically

> We would then define two sets of handlers for branch instructions
> 
> struct branch_dispatch {
>                 int           bytecode;           /* The bytecode */
>                 void       *handler;            /* The handler for it */
> };
> typedef struct branch_dispatch branch_dispatch;
> 
> branch_dispatch safe_branch_dispatch_table[] = {
>                 {              Bytecodes::_ifle,safe_ifle_handler },
>                 {              Bytecodes::_ifgt,safe_ifgt_handler },
>                 ...
> };
> 
> branch_dispatch unsafe_branch_dispatch_table[] = {
>                 {              Bytecodes::_ifle,unsafe_ifle_handler },
>                 {              Bytecodes::_ifgt,unsafe_ifgt_handler },
>                 ...
> }
> 
> notice_safepoints() and ignore_safepoints() then become
> 
> void update_table(branch_dispatch *p, branch_dispatch *pend)
> {
>                 do { opclabels_data[p->bytecode] = p->handler; } while (p++ < pend);
> }
> 
> notice_safepoints()
> {
>                 update_table(safe_branch_dispatch_table,
> 			safe_branch_dispatch_table +
> 			sizeof(safe_branch_dispatch_table)/sizeof(branch_dispatch));
> }
> 
> ignore_safepoint()
> {
>                 update_table(unsafe_branch_dispatch_table,
> 		unsafe_branch_dispatch_table +
> 		sizeof(unsafe_branch_dispatch_table)/sizeof(branch_dispatch));
> }

Hmm, this doesn't look like it's thread safe to me.  It would make more
sense to have a pointer to the branch dispatch table that's updated
atomically.

> Finally, can someone enlighten me as to the purpose of
> INCR_INVOCATION_COUNT, if we can get rid of that we could get rid of all
> backedge checks.

Surely the compile broker uses that count.

Andrew.