[14] RFR(S): 8236443 : Issues with specializing vector register type for phi operand with generic operands

[Vladimir Ivanov]

Hi Jatin,

> Webrev: http://cr.openjdk.java.net/~jbhateja/8236443/webrev.02/

I like that you put the check on use side (ShiftV + ideal_in_idx).

Though ShiftCntV def is the only viable option in practice (phis 
shouldn't occur on x86 and would trigger a bug due to mismatch between 
node type and ideal_reg), it's nice to see such case being ruled out.

Some comments:

=======================================================================
src/hotspot/cpu/x86/x86.ad:

+const uint Matcher::special_vector_ideal_reg(int ideal_reg, int opcode, 
int ideal_in_idx, int size) {

I don't see a compelling reason why it should be a platform-specific 
check. ShiftCntV nodes override Node::ideal_reg() and putting the check 
in shared code looks appropriate.

Considering how LShiftCntVNode::ideal_reg is defined [1], IMO calling 
Matcher::vector_shift_count_ideal_reg(t->length_in_bytes()) is better 
than hardcoding VecS ideal register (which is x86-specific).


=======================================================================
+    case Op_RShiftCntV:
+    case Op_LShiftCntV:
+      // Ideal index corrosponding to def operand.
+      if (ideal_in_idx == -1)
+        return Op_VecS;
+      break;

Shouldn't ideal_in_idx be 0 instead?


Some stylistic notes:

=======================================================================
src/hotspot/cpu/x86/x86.ad:

+MachOper* Matcher::specialize_vector_operand_helper(Node* m, MachNode* 
n, int op_idx) {

It's a bit confusing to see Node* named "m" and MachNode* named "n". In 
the code base it's usually the opposite: n stands for Node and m stands 
for MachNode.

But maybe "def" and "use" are better names?


=======================================================================

+  switch(opcode) {
+    default:
+      return ideal_reg;
+    case Op_LShiftVB:
...
+    case Op_URShiftVL:
+      if (ideal_in_idx == 2)
+        return Op_VecS;
+      break;
+    case Op_RShiftCntV:
+    case Op_LShiftCntV:
+      // Ideal index corrosponding to def operand.
+      if (ideal_in_idx == -1)
+        return Op_VecS;
+      break;
+  }

Does it make sense to introduce ShiftV and ShiftCntV node classes [2] 
and use Node::is_ShiftV()/is_ShiftCntV() checks instead?

=======================================================================

+  switch(opcode) {
+    default:
+      return ideal_reg;
...
+  }
+  return ideal_reg;

return statement is redundant.


=======================================================================

src/hotspot/share/opto/matcher.cpp:

-    MachNode* m = live_nodes.pop()->isa_Mach();
+    Node* n = live_nodes.pop();
+    if (!n->is_Mach())
+      continue;
+    MachNode * m = n->as_Mach();
      if (m != NULL) {


Node::isa_Mach() + != NULL check is a well-established convention in C2 
code base to perform type checks on nodes (is_Mach()+as_Mach()). I'd 
prefer to keep the original version.

Best regards,
Vladimir Ivanov

[1] class LShiftCntVNode : public VectorNode {
       virtual uint ideal_reg() const { return 
Matcher::vector_shift_count_ideal_reg(vect_type()->length_in_bytes()); }
     };

[2] 
http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/node.hpp#l716

> Generic operand processing has been skipped for non-machine nodes (e.g. Phi) since they are skipped by the matcher.
> Non-definition operand resolution of a machine node will be able to pull the type information from non-machine node.
> 
> Re-organized the code by adding a target specific routine which can be used to return the operand types for special ideal nodes e.g. RShiftCntV/LShiftCntV.
> For such nodes, definition machine operand vary for different targets and vector lengths, a type based generic operand resolution
> is not possible in such cases.
> 
> Best Regards,
> Jatin
> 
>