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

[Vladimir Kozlov]

Thank you, Vladimir, for comments and review.

Regards,
Vladimir K

On 9/9/20 7:57 AM, Vladimir Ivanov wrote:
> 
>>> 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