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

[Vladimir Kozlov]

Thank you, Jatin

On 9/3/20 10:08 PM, Bhateja, Jatin wrote:
> Hi Vladimir,
> 
> Thanks for taking care of this.
> 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.

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.

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.

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.

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

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