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

[Vladimir Ivanov]

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