RFR: 8271589: fatal error with variable shift count integer rotate operation. [v2]

[Jatin Bhateja]

On Tue, 3 Aug 2021 01:52:43 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>>> > I assume `TestLongVectRotate.java` should be also updated.
>>> > And you need add cases for Long type vector shifts if they can be generated.
>>> 
>>> Long rotates patterns with variable shifts i.e. Long.rotateLeft/Right(a[i], int_b[i]/ (int) long_b[i]) are not auto-vectorized currently.
>>> Problem is only seen with rotation over integers. I have kept the changes minimal as suggested earlier.
>> 
>> You said in an other comment:
>> "This is to handle long rotates, SLP will insert a ConvI2L before broadcasting shift value into a long vector."
>> 
>> Is it really a big change for test to cover this case? This is new code in your changes which needs test coverage I think.
>> 
>> I will start testing current changes (with removed assert(is_invariant_vector()) we agreed on).
>
>> > > I assume `TestLongVectRotate.java` should be also updated.
>> > > And you need add cases for Long type vector shifts if they can be generated.
>> > 
>> > 
>> > Long rotates patterns with variable shifts i.e. Long.rotateLeft/Right(a[i], int_b[i]/ (int) long_b[i]) are not auto-vectorized currently.
>> > Problem is only seen with rotation over integers. I have kept the changes minimal as suggested earlier.
>> 
>> You said in an other comment:
>> "This is to handle long rotates, SLP will insert a ConvI2L before broadcasting shift value into a long vector."
>> 
> Yes this comment is valid for Long.rotateLeft/Right(a[i], SHIFT/IMM_SHIFT) patterns.
> 
>> Is it really a big change for test to cover this case? This is new code in your changes which needs test coverage I think.
>> 
>> I will start testing current changes (with removed assert(is_invariant_vector()) we agreed on).

> @jatin-bhateja I don't understand why you don't want to add testing for `Long.rotateLeft/Right(a[i], SHIFT/IMM_SHIFT) `.
> Yes, this bug failed only for Int vectors. But you added code for Longs which have to be verified.
> 
> And you need second review.

Hi @vnkozlov ,
My apologies for not mentioning earlier that such test patterns (Long.rotateLeft/Right(a[i], scalar_shift/imm_shift) are already present in jdk/test/hotspot/jtreg/compiler/c2/cr6340864/TestLongVectRotate.java

-------------

PR: https://git.openjdk.java.net/jdk/pull/4956