EVALUATION

ChangeSet=http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/844a9d73ec22,ChangeRequest=6916644
                                     

EVALUATION

The use of an expand rule involving a match with a load triggered some previously unexercised code in the adlc which screwed up the edges when generating the node.  I suspect it's from a time when what the comment says was true but it's clear not true now.  The memory edge is inserted irrespective of type of memory op.  The fix is simply to remove the logic:

diff -r cf0685d550f1 src/share/vm/adlc/output_c.cpp
--- a/src/share/vm/adlc/output_c.cpp    Wed Jan 20 22:10:33 2010 -0800
+++ b/src/share/vm/adlc/output_c.cpp    Fri Jan 22 11:16:27 2010 -0800
@@ -1546,17 +1546,6 @@ void ArchDesc::defineExpand(FILE *fp, In
     // Build a mapping from operand index to input edges
     fprintf(fp,"  unsigned idx0 = oper_input_base();\n");

-    // The order in which inputs are added to a node is very
-    // strange.  Store nodes get a memory input before Expand is
-    // called and all other nodes get it afterwards so
-    // oper_input_base is wrong during expansion.  This code adjusts
-    // is so that expansion will work correctly.
-    bool missing_memory_edge = node->_matrule->needs_ideal_memory_edge(_globalNames) &&
-                               node->is_ideal_store() == Form::none;
-    if (missing_memory_edge) {
-      fprintf(fp,"  idx0--; // Adjust base because memory edge hasn't been inserted yet\n");
-    }
-
     for( i = 0; i < node->num_opnds(); i++ ) {
       fprintf(fp,"  unsigned idx%d = idx%d + num%d;\n",
               i+1,i,i);
@@ -1611,10 +1600,9 @@ void ArchDesc::defineExpand(FILE *fp, In
         int node_mem_op = node->memory_operand(_globalNames);
         assert( node_mem_op != InstructForm::NO_MEMORY_OPERAND,
                 "expand rule member needs memory but top-level inst doesn't have any" );
-        if (!missing_memory_edge) {
-          // Copy memory edge
-          fprintf(fp,"  n%d->add_req(_in[1]);\t// Add memory edge\n", cnt);
-        }
+
+        // Copy memory edge
+        fprintf(fp,"  n%d->add_req(_in[1]);\t// Add memory edge\n", cnt);
       }

       // Iterate over the new instruction's operands

The crash resulted for an object being cast to an unrelated type and then performing a virtual dispatch on it.  This rule seem to be the only place where this has ever been exercised in our system.
                                     

EVALUATION

Actually I'm incorrect about where the code came from.  It was introduced as part of the compressed oops work.  It doesn't appear to trigger at all these days so I'm not sure what problem it thinks is happening.  I'll have to investigate this more.  An alternate fix that it completely safe is to stop using the expand rule.

diff -r cf0685d550f1 src/cpu/x86/vm/x86_32.ad
--- a/src/cpu/x86/vm/x86_32.ad  Wed Jan 20 22:10:33 2010 -0800
+++ b/src/cpu/x86/vm/x86_32.ad  Fri Jan 22 11:24:41 2010 -0800
@@ -7841,9 +7841,10 @@ instruct cmovI_memUCF(cmpOpUCF cop, eFla
   predicate(VM_Version::supports_cmov() );
   match(Set dst (CMoveI (Binary cop cr) (Binary dst (LoadI src))));
   ins_cost(250);
-  expand %{
-    cmovI_memU(cop, cr, dst, src);
-  %}
+  format %{ "CMOV$cop $dst,$src" %}
+  opcode(0x0F,0x40);
+  ins_encode( enc_cmov(cop), RegMem( dst, src ) );
+  ins_pipe( pipe_cmov_mem );
 %}

 // Conditional move
                                     

EVALUATION

The problem is that the code to deal with a potentially missing memory edge isn't completely correct.  Sometimes loads will already have their memory edge connected and sometimes it won't.  It depends on what the full match tree looks like.  If the tree starts with a store then any loads in the tree will have their memory edge connected otherwise they won't.  The simplest fix is to pass the mem into expand so it can tell whether to handle the memory edge or not.