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

[Bhateja, Jatin]

Hi Vladimir,

Please find the update patch at below link.

http://cr.openjdk.java.net/~jbhateja/8236443/webrev.04/

Regards,
Jatin

> -----Original Message-----
> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> Sent: Friday, January 3, 2020 4:46 AM
> To: Bhateja, Jatin <jatin.bhateja at intel.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: [14] RFR(S): 8236443 : Issues with specializing vector register type
> for phi operand with generic operands
> 
> 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
> >
> >