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

[Bhateja, Jatin]

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