[14] RFR (L): 8235719: C2: Merge AD instructions for ShiftV, AbsV, and NegV nodes

[Vladimir Ivanov]

> My concern is only about vshiftL_arith_reg() for RShiftVL. It should use 
> 'if' instead of switch statement and supported length should be filtered 
> by match_rule_supported_vector(). Predicate should only check UseAVX <= 2.

Good point, Vladimir. Fully agree. Will make the adjustment before the push.

Best regards,
Vladimir Ivanov

> On 12/11/19 9:03 AM, John Rose wrote:
>> Thanks for taking my comments into account.
>> I looked again at your webrev and like it even better.
>> Any of the current micro-versions on the table is OK with me.
>>
>> — John
>>
>>> On Dec 11, 2019, at 2:31 AM, Vladimir Ivanov 
>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>
>>> Thanks for reviews, Vladimir and John.
>>>
>>> Updated version:
>>>   http://cr.openjdk.java.net/~vlivanov/jbhateja/8235719/webrev.01/
>>>
>>> On 11.12.2019 05:48, Vladimir Kozlov wrote:
>>>> In general I don't like using switches in this changes. In most 
>>>> examples you have only 2 instructions to choose from which could be 
>>>> done with 'if/else'. 'default: ShouldNotReachHere()' is big code if 
>>>> inlined and never will be hit - you should hit first checks in 
>>>> supported vector size code.
>>>
>>> Didn't have strong opinion about them (and still don't), so I 
>>> refactored most of the switches to branches.  Let me know how it 
>>> looks now.
>>>
>>> Regarding ShouldNotReachHere(): it would be unfortunate if we have to 
>>> take size code increase into account when using it to mark never-taken.
>>>
>>> Do you prefer "assert(false,...)" instead on for default case in 
>>> switches?
>>>
>>>> I may prefer to see 2 AD instructions as you had in previous changes.
>>>
>>> Considering the main motivation is to reduce the number of 
>>> instructions used, that would be a counter change. As I write later 
>>> to John, I would like to see the dispatching be hidden inside 
>>> MacroAssembler. It'll address your current concerns, right?
>>>
>>>> In vabsnegF() switch cases should be 2,8,16 instead of 2,4,8.
>>>
>>> Good catch! Fixed.
>>>
>>>> Why you need predicate for vabsnegD ? Other length is not supported 
>>>> anyway.
>>>
>>> Agree, fixed.
>>>
>>>
>>> On 11.12.2019 03:00, John Rose wrote:
>>>> Thank you, reviewed.
>>>>
>>>> For consistency, I’d expect to see the AD file mention vshiftq
>>>> instead of in or in addition to the very specific evpsraq.
>>>> Maybe actually call vshiftq (consistent with other parts of
>>>> AD) and comment that it calls evpsraq?  I had to look inside the
>>>> macro assembler to verify that evpsraq was properly aligned
>>>> with the other cases.  Or just leave a comment saying this is
>>>> what vshiftq would do also, like the other instructions.
>>>
>>> In general, I'd like to see all the hardware-specific dispatching 
>>> logic to be moved into MacroAssembler and AD file just to call into 
>>> them.
>>>
>>> But we (Jatin, Sandhya, and me) decided to limit the amount of 
>>> refactorings and upstream what Jatin ended up with.
>>>
>>> Are you fine with covering evpsraq case in a follow-up change?
>>>
>>>
>>>> For vabsnegF, I suggest adding a comment here:
>>>>
>>>> +  predicate(n->as_Vector()->length() == 2 ||
>>>> +            // case 4 is handled as a 1-operand instruction by 
>>>> vabsneg4F
>>>> +            n->as_Vector()->length() == 8 ||
>>>> +            n->as_Vector()->length() == 16);
>>>>
>>>
>>> I took slightly different route and rewrote it as follows:
>>>
>>> http://cr.openjdk.java.net/~vlivanov/jbhateja/8235719/webrev.01/individual/webrev.08.vabsneg_float/src/hotspot/cpu/x86/x86.ad.udiff.html 
>>>
>>>
>>> +instruct vabsnegF(vec dst, vec src, rRegI scratch) %{
>>> +  predicate(n->as_Vector()->length() != 4); // handled by 1-operand 
>>> instruction vabsneg4F
>>>
>>> instruct vabsneg4F(vec dst, rRegI scratch) %{
>>> +  predicate(n->as_Vector()->length() == 4);
>>>
>>> It looks clearer than the previous version.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>
>>>> Vladimir
>>>> On 12/10/19 2:29 PM, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/jbhateja/8235719/webrev.00/all
>>>>> https://bugs.openjdk.java.net/browse/JDK-8235719
>>>>>
>>>>> Merge AD instructions for the following vector nodes:
>>>>>     - LShiftV*, RShiftV*, URShiftV*
>>>>>     - AbsV*
>>>>>     - NegV*
>>>>>
>>>>> Individual patches:
>>>>>
>>>>> http://cr.openjdk.java.net/~vlivanov/jbhateja/8235719/webrev.00/individual 
>>>>>
>>>>>
>>>>> As Jatin described, merging is applied only to AD instructions of 
>>>>> similar shape. There are some more opportunities for 
>>>>> reduction/merging left, but they are deliberately left out for 
>>>>> future work.
>>>>>
>>>>> The patch is derived from the initial version of generic vector 
>>>>> support [1]. Generic vector support was reviewed earlier [2].
>>>>>
>>>>> Testing: tier1-4, test run on different CPU flavors (KNL, SKL, etc)
>>>>>
>>>>> Contributed-by: Jatin Bhateja <jatin.bhateja at intel.com>
>>>>> Reviewed-by: vlivanov, sviswanathan, ?
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> [1] 
>>>>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-August/034822.html 
>>>>>
>>>>>
>>>>> [2] 
>>>>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/036016.html 
>>>>>
>>