[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR: 8303045: Remove RegionNode::LoopStatus::NeverIrreducibleEntry assert with wrong assumption
From:       Emanuel Peter <epeter () openjdk ! org>
Date:       2023-02-27 7:18:20
Message-ID: wkJ0fjal9unCLWhUssgPo9VgpXpMgQ2_SQSO_43N9sI=.39a7e522-e59d-42a3-8831-a2860f896ebc () github ! com
[Download RAW message or body]

On Thu, 23 Feb 2023 10:43:50 GMT, Christian Hagedorn <chagedorn@openjdk.org> wrote:

> > I had to remove the first of these two asserts:
> > https://github.com/openjdk/jdk/blob/ac7119f0d5319a3fb44dc67a938c3e1eb21b9202/src/hotspot/share/opto/cfgnode.cpp#L438-L442
> >  
> > **Context**
> > 
> > The original idea for the two asserts was that we cannot accidentally lose \
> > information: The default value is `NeverIrreducibleEntry`, and once we change it \
> > to a different value (`Reducible` or `MaybeIrreducibleEntry`) we have no reason \
> > to ever go back to the default. 
> > However, for this, the second assert suffices: we should only set the value if it \
> > is still at default. 
> > **Details: explanation of regression test**
> > 
> > I have an example where the first assert fails, it is the regression test that is \
> > attached here. 
> > We inline
> > https://github.com/openjdk/jdk/blob/c6b4728177753513c87ee025a99f373e97de1466/test/ \
> > hotspot/jtreg/compiler/loopopts/TestInlinedSplitFallInIrreducibleLoopStatusMain.java#L45-L48
> >  
> > And in the inlined method, we have a region on which \
> > `IdealLoopTree::split_fall_in` is called: \
> > https://github.com/openjdk/jdk/blob/c6b4728177753513c87ee025a99f373e97de1466/test/ \
> > hotspot/jtreg/compiler/loopopts/TestInlinedSplitFallInIrreducibleLoopStatus.jasm#L53-L54
> >  
> > Since the region is in an inlined method, its loop status is still \
> > `NeverIrreducibleEntry` (we can only set it to `Reducible` if we are absolutely \
> > sure there is no enclosing irreducible loop. And we only want to set \
> > `MaybeIrreducibleEntry` if we have to, because then we have to do additional work \
> > when an entry is lost). Now, when we do `split_fall_in`, we copy the loop status \
> > from the `_head` region to the new `landing_pad`. So we essentially did \
> > `set_loop_status(NeverIrreducibleEntry)`, which triggered the assert. \
> > https://github.com/openjdk/jdk/blob/c6b4728177753513c87ee025a99f373e97de1466/src/hotspot/share/opto/loopnode.cpp#L3144
> >  
> > ![image_480](https://user-images.githubusercontent.com/32593061/220640119-8fe59143-bd6e-42fe-8df0-1b01f1e87fb5.png)
> >  Here the graph of the test, before we call `split_fall_in`. Red: `test_outer`. \
> > Orange: `test_inner`. Yellow: the loop inside `test_inner`, with `_head == 78 \
> > Region`.
> 
> That sound reasonable. Looks good!

Thanks @chhagedorn @vnkozlov for the reviews!

-------------

PR: https://git.openjdk.org/jdk/pull/12713


[prev in list] [next in list] [prev in thread] [next in thread]