RFR (S): 8021898: Broken JIT compiler optimization for loop unswitching

[Vladimir Kozlov]

Thank you, Niclas

http://cr.openjdk.java.net/~kvn/8021898/webrev.01/

On 8/14/13 7:47 AM, Niclas Adlertz wrote:
> Hi Vladimir,
>
> The changes look good. There are some minor things:
>
> Due to a recent patch, _bbs is not accessible from outside PhaseCFG, so instead of:
> Block* borig = _cfg._bbs[orig->_idx];
> you should do:
> Block* borig = _cfg.get_block_for_node(orig);

Done. I also changed _cfg._bbs.map() to _cfg.map_node_to_block().
I prepared this fix before you pushed your changes. Should have updated 
it with latest sources.

>
> Also, shouldn't the proj nodes always follow immediately after the orig node?
> If so, instead of doing:
> while (proj->in(0) == orig && proj->is_MachProj()) {
> we could just do:
> while (proj->is_MachProj()) {
>    assert(proj->in(0) == orig, "the projection node should originate from orig");

I have my reservations about gcm/lcm and RA. I rewrote the code to go 
through outputs of orig node instead. This way we guarantee to find all 
related MachProj nodes. And I added more asserts.

>
> Also, we could adjust the declaration and definition of clone_projs:
> from:
> int PhaseChaitin::clone_projs(Block *b, uint idx, Node *orig, Node *copy, uint &max_lrg_id) {
> to:
> int PhaseChaitin::clone_projs(Block* b, uint idx, Node* orig, Node* copy, uint& max_lrg_id) {

Done.

Thanks,
Vladimir

>
> Kind Regards,
> Niclas Adlertz
>
>
> On 14 aug 2013, at 01:15, Vladimir Kozlov <Vladimir.Kozlov at oracle.com> wrote:
>
>> http://cr.openjdk.java.net/~kvn/8021898/webrev/
>>
>> The problem is not in loop unswitching. Loop unswitching creates particular code shape (partialSubtypeCheck_vs_Zero mach node followed by cmovI_reg node) with which we hit the problem in RA.
>>
>> RA clones the node producing flag (partialSubtypeCheck_vs_Zero) and place it near user (cmovI_reg) in split_Rematerialize(). When a node is cloned RA also should clone related MachProj nodes which indicate KILLed flags and registers (effect(KILL rcx, KILL result)). The problem is RA clones only one MachProj node in  PhaseChaitin::clone_projs_shared().
>>
>> The fixed is to clone all related MachProj nodes. The method clone_projs() returns number of cloned MachProj nodes.
>>
>> The increment of max_lrg_id is moved inside clone_projs_shared() which is renamed to clone_projs() since we don't need second version of clone_projs().
>>
>> I also fixed/modified output in hs_err file in case VM crash in compiled code.
>>
>> Compiled frame output now has compile_id, compiler name, bytecode size, and offset in the code is hex:
>>
>> # J 61 C2 org.apache.http.impl.cookie.BestMatchSpec.formatCookies(Ljava/util/Lis
>> t;)Ljava/util/List; (116 bytes) @ 0xfffffd7ff8f26907 [0xfffffd7ff8f25340+0x15c7]
>>
>> before:
>>
>> # J org.apache.http.impl.cookie.BestMatchSpec.formatCookies(Ljava/util/List;)Ljava/util/List; @ 0xfffffd7ff8f2a837 [0xfffffd7ff8f29280+5559]
>>
>> I also added java frames prints in "Native frames:" during error report (hs_err). Before it printed only last compiled frame and stopped because C2 compiled frame trash EBP. Note: hs_err file does not have "Java frames:" in such case because it is guarded by has_last_Java_frame() and error reporting is called from signal processing code so last frame is native, I assume it is the reason - I did not investigated it in deap.
>>
>> Thanks,
>> Vladimir
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>