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

[Vladimir Kozlov]

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.

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 ) ) );


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?

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.


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?

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


Thanks,
Vladimir

John Rose wrote:
> In order to reduce the size of my forthcoming method handles changeset, 
> I have (at Vladimir's suggestion) pulled out chunks of independent 
> changes this week.  The four chunks amount to about 40% of the changes, 
> and are more easily tested and reviewed in isolation.
> 
> The fourth chunk 6814659 is the most miscellaneous.  It is a bunch of 
> changes that were necessary either for debugging method handles, or for 
> supporting them.  They can all be considered independently.
> 
> Here are the changes:
>   http://cr.openjdk.java.net/~jrose/6814659/webrev.00/
> 
> Not much review is necessary here, because these changes have already 
> been reviewed as part of the larger changeset.
> 
> The current draft of the big change is here:
>   http://cr.openjdk.java.net/~jrose/6655638/webrev.00/
> 
> -- John
> 
> P.S. For the record, here is the sequence:
> 
> 1. 6812678: macro assembler needs delayed binding of a few constants 
> (for 6655638)
>   http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/56aae7be60d4
> 
> 2. 6812831: factor duplicated assembly code for megamorphic 
> invokeinterface (for 6655638)
>   http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/9adddb8c0fc8
> 
> 3. 6812831 factor general subclass check (for 6655638)
>   http://cr.openjdk.java.net/~jrose/6813212/webrev.01/  (not yet integrated)
> 
> 4. 6814659: separable cleanups and subroutines for 6655638
>   http://cr.openjdk.java.net/~jrose/6814659/webrev.00/  (not yet integrated)
> 
> 5. 6655638: dynamic languages need method handles
>   http://cr.openjdk.java.net/~jrose/6655638/webrev.00/  (not yet integrated)
> 
> And here are the patch file sizes (from my mq patch queue):
> 
>     1178    5493   48571 asm-6812678.patch
>      979    5029   40958 ilookup-6812831.patch
>     1611    7834   70518 subtype-6813212.patch
>     1147    4962   48089 minor-6814659.patch
>     7274   33505  311948 meth.patch
>    12189   56823  520084 total
>