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

Fri Sep 11 23:33:47 UTC 2020

Thank you, Tobias

Vladimir K

On 9/9/20 10:45 PM, Tobias Hartmann wrote:
> +1
> 
> Best regards,
> Tobias
> 
> On 10.09.20 07:37, 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