[16] RFR(M) 8252188: Crash in OrINode::Ideal(PhaseGVN*, bool)+0x8b9

[Vladimir Ivanov]

>> Similar strict check for constant shift is needed in OrVNode::Ideal 
>> routine in vectornode.cpp.
> 
> It took me some time to analyze your code for "lazy de-generation" of 
> Rotate vectors. As I understand you want to preserve scalar optimization 
> which creates Rotate nodes but have to revert it to keep vectorization 
> of java code.

Yes, the main motivation was to simplify the implementation: keep 
vectorization logic simple (just Rotate -> RotateV transformation) and 
don't mess with matching if RotateV is not supported (expand ideal nodes 
instead of adding special AD instructions).

> Method degenerate_vector_rotate() has is_Con() check and, in general, it 
> could be TOP because we do loop optimizations after vectorization. I 
> added isa_int() check and treat 'cnt' in other case as variable to do 
> transformation on 'else' branch and let sub-graph collapse there.
> I also refactor degenerate_vector_rotate() to make it compact.

Good point.

> Second, about OrVNode::Ideal(). I am not sure how safe it is without 
> additional investigation because currently it is not executed. Based on 
> comment it was added for VectorAPI which is experimental and not pushed 
> yet.
> The code is convoluted and does not match scalar Or::Ideal() code.
> 
> OrINode::Ideal() does next checks for left rotation:
>    if (Matcher::match_rule_supported(Op_RotateLeft) &&
>        lopcode == Op_LShiftI && ropcode == Op_URShiftI && in(1)->in(1) 
> == in(2)->in(1)) {
> 
> but OrVNode::Ideal() does:
>    if (Matcher::match_rule_supported_vector(Op_RotateLeftV, vec_len, bt) &&
>        ((ropcode == Op_LShiftVI && lopcode == Op_URShiftVI) ||
> 
> Why it checks RIGHT operator for LShiftV????
> 
> And asserts are contradicting:
>     assert(Op_RShiftCntV == in(1)->in(2)->Opcode(), "LShiftCntV operand 
> expected");
> 
> Was this code tested? My immediate reaction is simple delete it now and 
> add reworked and tested version back with EnableVectorSupport flag check 
> after VectorAPI is integrated.

Sounds good.

My feedback on OrVNode::Ideal() during 8248830 review was:

"> 6) Constant folding scenarios are covered in RotateLeft/RotateRight 
idealization, inferencing of vector rotate through OrV idealization 
covers the vector patterns generated though non SLP route i.e. VectorAPI.

I'm fine with keeping OrV::Ideal(), but I'm concerned with the general 
direction here - duplication of scalar transformations to lane-wise 
vector operations. It definitely won't scale and in a longer run it 
risks to diverge. Would be nice to find a way to automatically "lift" 
scalar transformations to vectors and apply them uniformly. But right 
now it is just an idea which requires more experimentation."

> Reworked version may use the same new rotate_shift() I added. I start 
> rewriting it but since I can't test it and I am not sure may be edges 
> are swapped indeed. I am suggesting to remove it.
> 
> Also VectorAPI should use Rotate vectors from start which we can 
> de-generation if not supported. So I am not sure how OrVNode::Ideal() 
> will be usefull for VEctorAPI too.

Though the API exposes rotation as a dedicated operation, users are free 
to code it explicitly with vector shifts and vector or. Basically, the 
situation is similar to scalar case: there are dedicated methods 
available (Long.rotateLeft/rotateRight), but users are free to code 
their own variant. And sometimes more general code shapes may degenerate 
into rotates (as a result of other optimizations).

And from Vector API implementation perspective, it is attractive to 
implement vector rotation purely in Java code as a composition of vector 
shifts/or operations rather than using JVM intrinsic for it.

So, there's a number of use cases when transformations on vector nodes 
becomes profitable.

> http://cr.openjdk.java.net/~kvn/8252188/webrev.01/

Looks good.

Best regards,
Vladimir Ivanov

> 
> About testing. I see you used a lot of -128, 128 and similar values 
> which are larger then bits in Java Integer and Long.
> But Java do masking of shift count by default before executing shift.
> I would prefer if something like 31 (or 63 for Long) were used instead. 
> Otherwise Rotate vectors are not generated and tested.
> 
> compiler/intrinsics/TestRotate.java calls verify() after each operation 
> as result it is really hard to see generated assembler. I think we 
> should at least exclude inlinining of verify().
> 
> I will work on tests and have an other update.
> 
> Thanks,
> Vladimir K
> 
> 
>>
>> Regards,
>> Jatin
>>
>>> -----Original Message-----
>>> From: hotspot-compiler-dev 
>>> <hotspot-compiler-dev-retn at openjdk.java.net> On
>>> Behalf Of Vladimir Kozlov
>>> Sent: Friday, September 4, 2020 3:14 AM
>>> To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
>>> Subject: [16] RFR(M) 8252188: Crash in OrINode::Ideal(PhaseGVN*,
>>> bool)+0x8b9
>>>
>>> https://cr.openjdk.java.net/~kvn/8252188/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8252188
>>>
>>> Code added by 8248830 [1] uses Node::is_Con() check when looking for
>>> constant shift values.
>>> Unfortunately it does not guarantee that it will be Integer constant
>>> because TOP node is also ConNode.
>>> I used C2 types to check and get shift values. I also refactor code to
>>> consolidate checks.
>>>
>>> Tested: tier1, hs-tier2, hs-tier3.
>>> Verified fix with replay file from bug report.
>>> I also checked that RotateBenchmark.java added by 8248830 still creates
>>> Rotate vectors after this fix.
>>>
>>> I created subtask to add new regerssion test later because this fix is
>>> urgent and I did not have time to prepare it.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8248830