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

[Vladimir Kozlov]

Thank you, Jatin

Vladimir K

On 9/9/20 10:37 PM, Bhateja, Jatin wrote:
> Hi VladimirK,
> Removing OrVNode::Ideal looks correct, it was added to cover vector API use case.
> 
> Regards,
> Jatin
> 
>> -----Original Message-----
>> From: hotspot-compiler-dev <hotspot-compiler-dev-retn at openjdk.java.net> On
>> Behalf Of Vladimir Kozlov
>> Sent: Wednesday, September 9, 2020 11:17 PM
>> To: hotspot-compiler-dev at openjdk.java.net
>> Subject: Re: [16] RFR(M) 8252188: Crash in OrINode::Ideal(PhaseGVN*,
>> bool)+0x8b9
>>
>> 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