gmane.comp.java.openjdk.hotspot.compiler.devel

Re: Request for reviews (M): 7170463: C2 should recognize"obj.getClass() == A.class" code pattern

Krystal Mok — 2012-05-24T08:28:27

Hi Vladimir and John,

Thanks for polishing the changes!
Comments inline below:

On Thu, May 24, 2012 at 1:02 PM, John Rose <john.r.rose-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org> wrote:

for the catch!


of sharpen_type_after_if() with the new and old code mixed together; now it
looks nice.



Thanks,
Kris

Re: Request for reviews (M): 7170463: C2 should recognize"obj.getClass() == A.class" code pattern

John Rose — 2012-05-24T05:02:47


That's fine; I like it.

New question:  Can the following call return Top?
  obj_type->join(con_type)

E.g.,
  Number x = 5;
  if (x.getClass() == String.class)
    System.out.println("fail!");

Perhaps the compare will short-circuit before that happens.  If it doesn't, we'll get assertion failures or worse.

— John

Re: Request for reviews (M): 7170463: C2 should recognize"obj.getClass() == A.class" code pattern

Vladimir Kozlov — 2012-05-23T23:47:00

I updated webrev:

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

I found additional issue with original changes. They accidentally removed 
optimization which replaces LoadKlass node with ConP (klass type) in map. Next 
code will not be executed:

    cast = con;  // Replace non-constant val by con.

New code should do separate map update of object which klass is compared.

John Rose wrote:

Done.


Base class TypeOopPtr is java pointer type and it currently includes 
TypeKlassPtr type until PermGen is removed. So for now I leave these checks as 
it is and late we can replace them with tp->isa_oopptr().

Vladimir

Re: RFR (M): 7023898: IntrinsifyAtomicLongFieldUpdater.getAndIncrement()

Roland Westrelin — 2012-05-23T14:19:08

Hi John,

For 1. et 2. below:


You're commenting this code change:

2866   if ((kind == xchg || kind == xadd) && type == T_LONG) {
2867     push_pair(load_store);
2868   } else {
2869     push(load_store);
2870   }

right?

Then for 2.: when kind == cmpxchg, then the else part is executed so load_store is pushed as a result.

and for 1.: when type is T_LONG for xchg or xadd, a pair is pushed but for cmpxchg a single result is pushed. So push_node wouldn't work, right?


Ok.


Ok.

Roland.

Re: Request for reviews (XS): 7170145: C1 doesn't respect the JMMwith volatile field loads

Roland Westrelin — 2012-05-23T08:49:16

Looks ok to me.

Roland.

Re: Request for reviews (XS): 7170145: C1 doesn't respect the JMMwith volatile field loads

Christian Thalinger — 2012-05-23T02:11:15

On May 22, 2012, at 6:30 PM, Mikael Vidstedt wrote:


Apparently I had too many versions of that text.  Thanks.

Re: Request for reviews (XS): 7170145: C1 doesn't respect the JMMwith volatile field loads

Mikael Vidstedt — 2012-05-23T01:30:26

Two typos:

145 // This is actually too strict and the JMM doesn't requires
146 // this is all cases (e.g. load a; volatile store b; load a)

should probably be "JMM doesn't _require_ this _in_ all cases...".

Cheers,
Mikael


On 2012-05-22 17:35, Christian Thalinger wrote:

Re: Request for reviews (XS): 7170145: C1 doesn't respect the JMMwith volatile field loads

Vladimir Kozlov — 2012-05-23T01:23:51

Good.

Vladimir

On 5/22/12 5:35 PM, Christian Thalinger wrote:

Request for reviews (XS): 7170145: C1 doesn't respect the JMM withvolatile field loads

Christian Thalinger — 2012-05-23T00:35:07

http://cr.openjdk.java.net/~twisti/7170145

7170145: C1 doesn't respect the JMM with volatile field loads
Reviewed-by:

ValueNumberingVisitor::do_LoadField does not include logic for
volatile fields which allows CSE of normal field loads across volatile
field loads.  That's explicitly prohibited by the JMM.

This patch also kills the memory across volatile field stores even it
is too strict for now because of volatile field stores and possible
future optimizations.

src/share/vm/c1/c1_ValueMap.hpp

Re: Request for reviews (M): 7170463: C2 should recognize"obj.getClass() == A.class" code pattern

Vladimir Kozlov — 2012-05-22T19:06:47

Thank you, John

I will try your suggestions.

Vladimir

John Rose wrote:

Re: Request for reviews (M): 7170463: C2 should recognize"obj.getClass() == A.class" code pattern

John Rose — 2012-05-22T18:30:07


Thanks, Kris and Vladimir, for pushing on this, and for dealing with a large number of comments.

This new version of the code looks robust and well-structured, except for a couple of small points, which I think are easily addressed.

This line:
  assert(ldk != NULL, "should have found a LoadKlass or LoadNKlass node")
could better protect the subsequent code as:
  assert(ldk != NULL && ldk->is_Load(), "should have found a LoadKlass or LoadNKlass node")

(It is unusual, though valid, to say 'tp->isa_instptr() || tp->isa_aryptr()'.  This is done twice in the new code.  I don't see a better way to go, although I note that ciObject has is_java_ptr, and I thought we used to have TypePtr::isa_javaptr.  Perhaps we should put it in type.hpp?)

The 'rep = obj / rep = val' stuff at the top of 'sharpen_type_after_if' is reasonable, but I'm uncomfortable with the mixed use of 'rep' and 'val' at the bottom.  Redundant information attracts bugs.  I would prefer to kill 'val' after 'rep' is defined, perhaps as:
  debug_only(val = (Node*)badAddress);
Later on, only rep should be used.  This will fix a probable bug at 'case BoolTest::ne'.

Or, better yet, get rid of rep/trep and say:
+   if (is_klass_cmp) {
+     tval = gvn.type(obj);
+     val = obj;
+   }
(If the side effect to the parameters is confusing, call them val0, tval0.)

Once this is done, the code for 'case BoolTest::eq' should factor a little bit better.  'tkp' is another bit of redundant information; I would rather see tcon used throughout.  This also will allow the 'eq' case to factor better.

Perhaps this should be done, to make the treatment of tval/val more consistent:

+   if (is_klass_cmp) {
+     tval = gvn.type(obj);
+     val = obj;
+     tcon = tcon->is_klassptr()->as_instance_type();
+     con = NULL;
+   }

This is stylistic stuff, but it affects bug tail, as in the case of the BoolTest::ne case and any other cases we may add later.

— John

Re: RFR (M): 7023898: IntrinsifyAtomicLongFieldUpdater.getAndIncrement()

Roland Westrelin — 2012-05-22T08:07:44

John,


Thanks for reviewing it. I will implement your suggestions.

Roland.

Request for reviews (M): 7170463: C2 should recognize "obj.getClass()== A.class" code pattern

Vladimir Kozlov — 2012-05-21T22:36:15

http://cr.openjdk.java.net/~kvn/7170463/webrev

7170463: C2 should recognize "obj.getClass() == A.class" code pattern

The idea is to optimize this code pattern:

   x.getClass() == A.class

into

   x._klass == klassof(A)

which shortens the original pattern by 1 load.

Tested with NSK, compiler regression tests, CTW.

Contributed-by: Krystal Mok <sajia-3b8fjiQLQpfQT0dZR+AlfA< at >public.gmane.org>

Re: inliner heuristics

Xin Tong — 2012-05-21T22:22:12

I am also looking at inlining of a polymorphic site. Why is there no guard
inserted which encloses the method that is inlined ? The receiver objects
could vary.

   // Try using the type profile.
    if (call_is_virtual && site_count > 0 && receiver_count > 0) {
      // The major receiver's count >= TypeProfileMajorReceiverPercent of
site_count.
      bool have_major_receiver = (100.*profile.receiver_prob(0) >=
(float)TypeProfileMajorReceiverPercent);
      ciMethod* receiver_method = NULL;
      if (have_major_receiver || profile.morphism() == 1 ||
          (profile.morphism() == 2 && UseBimorphicInlining)) {
        // receiver_method = profile.method();
        // Profiles do not suggest methods now.  Look it up in the major
receiver.
        //  the look up is performed on the profile.receiver(0). The first
receiver is the one mostly seen.
        receiver_method =
call_method->resolve_invoke(jvms->method()->holder(),
                                                      profile.receiver(0));
      }
      if (receiver_method != NULL) {
        // The single majority receiver sufficiently outweighs the minority.

*        // Should there be a guard insert to compare the receiver object
class with the inlined method class ?*

        CallGenerator* hit_cg = this->call_generator(receiver_method,
              vtable_index, !call_is_virtual, jvms, allow_inline,
prof_factor);
        if (hit_cg != NULL) {
          // Look up second receiver.
          CallGenerator* next_hit_cg = NULL;
          ciMethod* next_receiver_method = NULL;

          if (profile.morphism() == 2 && UseBimorphicInlining) {
            // XIN TONG, the second virtual call inlined.
            next_receiver_method =
call_method->resolve_invoke(jvms->method()->holder(),

 profile.receiver(1));
            if (next_receiver_method != NULL) {

*        // Should there be a guard insert to compare the receiver object
class with the inlined method class ?*
*
*
              next_hit_cg = this->call_generator(next_receiver_method,
                                  vtable_index, !call_is_virtual, jvms,
                                  allow_inline, prof_factor);
              if (next_hit_cg != NULL && !next_hit_cg->is_inline() &&
                  have_major_receiver && UseOnlyInlinedBimorphic) {
                  // Skip if we can't inline second receiver's method
                  next_hit_cg = NULL;
              }
            }


On Mon, May 21, 2012 at 3:14 PM, Xin Tong <xerox.time.tech-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

Re: debug in hotspot

Xin Tong — 2012-05-21T21:30:44


There is no way to force the compilation with C1/C2 on a
per-method granularity. But methods are compiled with C1 and then C2 in a
tiered compilation , right ?

Xin

inliner heuristics

Xin Tong — 2012-05-21T19:14:13

I am investigating the inliner heuristics in C2 compiler of hotspot. There
are a few things i do not get.

1. The inliner heuristics depend on 2 parameters - the call_site_count and
the invocation_count. The invocation_count is the number of times the
method is executed in the previous tier. i.e. interpreter for tier 1
compilation and tier 1 for tier 2 compilation. But what does
call_site_count represent. the number of callsites of this method within
the all callers of the method ? It seems it is calculated based on MD0,
what is the method data ?

2. Is it true that in theory all methods can be inlined in Java ? how about
in the hotspot JVM ?

Thanks

Xin

Re: RFR (M): 7023898: IntrinsifyAtomicLongFieldUpdater.getAndIncrement()

John Rose — 2012-05-21T16:29:20


The C2 part looks good.  I have a few small suggestions.

1. Replace push(cas) by push_node(type, load_store).

2. I don't understand why the (kind = cmpxchg) case doesn't push a result.  Does't the original push(cas) do that?

3. In the if/else chain testing 'kind', change the bare 'else' to 'else if (kind ≡ cmpxchg)' and follow up with 'else ShouldNotReachHere()'.  At least put an "assert(kind ≡ cmpxchg)".  This will ease maintenance, in case somebody adds a new case to load_store_kind.

4. Naming:  Consider using a more type-like name for load_store_kind, such as LoadStoreKind.  Compare ReexecuteState and NodeType enums.  Also, the enum names themselves could use a prefix or camel-case, e.g., LS_xadd or _xadd or XAdd.  Since the names are private to library_call.cpp, it doesn't matter as much as in other places, but it still improves readability of code that contains the names.  (Reason:  an all-lower-case simple name like xadd appears at first glance to be a local variable.  Manifest constants should look different.)

Overall, it is a nicely rounded extension, with good reuse of previously existing code.  Thanks!

— John

hg: hsx/hotspot-comp/hotspot: 7169934: pow(x,y) or x64 computes incorrect result when x<0 and y is an odd integer

2012-05-21T11:20:18

Changeset: e2961d14584b
Author:    roland
Date:      2012-05-21 09:46 +0200
URL:       http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/e2961d14584b

7169934: pow(x,y) or x64 computes incorrect result when x<0 and y is an odd integer
Summary: bad test for parity of y in pow(x,y) (c1, interpreter)
Reviewed-by: kvn, twisti

! src/cpu/x86/vm/assembler_x86.cpp

Re: RFS (XS): 7169934: pow(x,y) or x64 computes incorrect result when x<0 and y is an odd integer

Roland Westrelin — 2012-05-21T07:39:54

Thanks Vladimir & Christian.

Roland.

Re: Request for review (XS): crash in C2 when using-XX:+CountCompiledCalls

Krystal Mok — 2012-05-19T02:42:34

Thanks Chris and Vladimir!

Kris

On 2012-5-19, at 3:40, Vladimir Kozlov <vladimir.kozlov-QHcLZuEGTsvQT0dZR+AlfA< at >public.gmane.org> wrote:

hg: hsx/hotspot-comp/hotspot: 7170053: crash in C2 when using-XX:+CountCompiledCalls

2012-05-18T23:31:51

Changeset: cdd249497b34
Author:    twisti
Date:      2012-05-18 12:20 -0700
URL:       http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/cdd249497b34

7170053: crash in C2 when using -XX:+CountCompiledCalls
Reviewed-by: kvn, twisti
Contributed-by: Krystal Mok <sajia-3b8fjiQLQpfQT0dZR+AlfA< at >public.gmane.org>

! src/share/vm/opto/doCall.cpp