for (re-)review (M): 6814659: separable cleanups and subroutines for 6655638

[John Rose]

On Mar 12, 2009, at 5:19 PM, Vladimir Kozlov wrote:

> About next:
>
> > 3. 6812831 factor general subclass check (for 6655638)
> >   http://cr.openjdk.java.net/~jrose/6813212/webrev.01/  (not yet  
> integrated)
>
> Assembler code on x86 and sparc looks good for me.

Great.

It tests well on JPRT and CTW (all jars, several solaris configs in  
x86/sparc c1/c2 32/64).

I am confident there are fewer bugs after the change than before,  
despite the amount of touches.

> General question: why we do next check in enc_PartialSubtypeCheck()
>
>    // Compare super with sub directly, since super is not in its own  
> SSA.
>    // The compiler used to emit this test, but we fold it in here,
>    // to allow platform-specific tweaking on sparc.
>    __ cmpptr(Rrax, Rrsi);
>    __ jcc(Assembler::equal, hit);
>
> and in Sparc::partial_subtype_check()
>
>     // Compare super with sub directly, since super is not in its  
> own SSA.
>     // The compiler used to emit this test, but we fold it in here,
>     // to increase overall code density, with no real loss of speed.
>     { Label L;
>       __ cmp(O1, O2);
>       __ brx(Assembler::notEqual, false, Assembler::pt, L);
>       __ delayed()->nop();
>       __ retl();
>       __ delayed()->addcc(G0,0,O0); // set Z flags, zero result
>       __ bind(L);
>     }
>
> When in GraphKit::gen_subtype_check we have the check contrary to  
> the above
> comments:
>
>  // Check for self.  Very rare to get here, but its taken 1/3 the  
> time.
>  // No performance impact (too rare) but allows sharing of secondary  
> arrays
>  // which has some footprint reduction.
>  Node *cmp3 = _gvn.transform( new (C, 3) CmpPNode( subklass,  
> superklass ) );
>  Node *bol3 = _gvn.transform( new (C, 2) BoolNode( cmp3,  
> BoolTest::eq ) );
>  IfNode *iff3 = create_and_xform_if( control(), bol3,  
> PROB_LIKELY(0.36f), COUNT_UNKNOWN );
>  r_ok_subtype->init_req(2, _gvn.transform( new (C, 1) IfTrueNode  
> ( iff3 ) ) );
>  set_control(               _gvn.transform( new (C, 1)  
> IfFalseNode( iff3 ) ) );

It's an old piece of history, I guess, from when two engineers were  
working on one chunk of code.  Thanks for asking.  The extra self- 
check on the slow path apparently has been useless for years.

So, I have removed those extra checks, of course after checking all  
the usages of PartialSubtypeCheck nodes on x86 and sparc.

I put in new comments in graphKit and assembler_<arch> explaining why  
the self-check is there, and why it comes in a different order in C2  
than elsewhere.

Nice!

> Next, why we keep generate_partial_subtype_check stub?
> The only place which calls it is sparc.ad:
>
>  enc_class enc_PartialSubtypeCheck() %{
>    MacroAssembler _masm(&cbuf);
>    __ call(StubRoutines::Sparc::partial_subtype_check(),  
> relocInfo::runtime_call_type);
>    __ delayed()->nop();
>  %}
>
> Is there a reason to keep the call instead of using your new
> macroassembler methods?

Code density.  The sparc loop is much bigger than the x86 loop, so we  
out-of-line it.

The graphKit.cpp comments mention the issue of code size, and I  
improved them.

> assembler_x86.cpp check_klass_subtype_slow_path()
> Move next assert:
> +   assert(secondary_supers_addr.base() != rax, "put sub_klass in  
> another reg");
> (why not assert(sub_klass != rax, "put sub_klass in another reg") ?)
>
> before this code:
> +   if (super_klass != rax || UseCompressedOops) {
> +     if (!IS_A_TEMP(rax)) { push(rax); pushed_rax = true; }
> +     mov(rax, super_klass);
> +   }
>
> Also sub_klass != rcx since rcx is destroyed by debug counter code.

Good call.  Done.  I put the checks just before the instructions which  
would kill sub_klass with a bad assignment, rather at the very top of  
the function, where it would be hard to see why those restrictions  
matter.

> You changed encode_heap_oop(rax) to encode_heap_oop_not_null(rax).
> Can you give guaranty that super_klass value is not NULL for all  
> cases?

Yes, it would be fundamental system bug if we were to class-check  
against a null klassOop.  I put an explanatory comment near the calls  
to encode_heap_oop_not_null.

> x86_64.ad, x86_32.ad
>  You can remove the label 'cmiss' from enc_PartialSubtypeCheck.

Done.  I also removed the label 'hit' everywhere, since it is not used  
except by the self-checks, which I removed.

I've updated the webrev:
   http://cr.openjdk.java.net/~jrose/6813212/webrev.02/
   http://cr.openjdk.java.net/~jrose/6813212/01-to-02.patch.txt

> Thanks,
> Vladimir

Many thanks!  I owe you a drink of your favorite beverage.

-- John