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

[Vladimir Ivanov]

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

Looks good.

Test results are clean (hs-precheckin-comp,hs-tier1,hs-tier2).

Best regards,
Vladimir Ivanov

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