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

[John Rose]

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