RFR(S): 8243670: Unexpected test result caused by C2 MergeMemNode::Ideal

[Yangfei (Felix)]

Hi Tobias,

> -----Original Message-----
> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
> Sent: Wednesday, June 17, 2020 9:26 PM
> To: Yangfei (Felix) <felix.yang at huawei.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: RFR(S): 8243670: Unexpected test result caused by C2
> MergeMemNode::Ideal
> 
> Hi Felix,
> 
> On 17.06.20 14:30, Yangfei (Felix) wrote:
> > Thanks for confirming that.   Yes, this works for the reduced test case.
> > I see phi_base was calculated from base_memory().  That is in(AliasIdxBot))
> which is a "wide" memory state containing all alias categories.
> > So I was thinking that maybe the condition " phi_base->adr_type()-
> >higher_equal(phi_mem->adr_type())" will always equals false?
> > If that is true, then this is the same in functionality with my initial patch.
> Could you please clarify?
> 
> Right, that wouldn't work. What I was trying to suggest is to replace the
> MergeMem by the Phi with the most restrictive type if all inputs are Phis. I
> think that would be an Identity optimization and only work with MergeMems
> that have two inputs. For example, in your case it should be safe to replace
> the 4: MergeMem by 1: Phi1, right?

For the first iteration on the loop, the MergeMem node actually have three phis as input.
It looks like:
(gdb) p this->dump()
 556    MergeMem        === _  1  660  655  1  657  [[ 559 ]]  { N655:rawptr:BotPTR - N657:TestReplaceEquivPhis+12 * }  Memory: @BotPTR *+bot, idx=Bot; !orig=151 !jvms: TestReplaceEquivPhis::test @ bci:25
$2 = void
(gdb) p in(2)->dump()
 660    Phi     ===  679  752  583  [[ 556  533 ]]  #memory  Memory: @BotPTR *+bot, idx=Bot;
$3 = void
(gdb) p in(3)->dump()
 655    Phi     ===  679  752  583  [[ 556 ]]  #memory  Memory: @rawptr:BotPTR, idx=Raw;
$4 = void
(gdb) p in(5)->dump()
 657    Phi     ===  679  752  583  [[ 556  516 ]]  #memory  Memory: @TestReplaceEquivPhis+12 *, name=iFld, idx=5;
$5 = void

After the first iteration, in(3) (i.e., in(AliasIdxRaw)) was removed from the inputs. 
I am not sure if this transformation is correct even through it does not make a difference on app behavior. 
After that, the MergeMem have two phi nodes as input: in(5) and in(2) ((i.e., in(AliasIdxBot))).  This is the same structure as described in my first email. 

Here, I see in(2) is also an input for node 533: 
(gdb) p in(2)->find(533)->dump() 
 533    LoadI   === _  660  180  [[ 517  532  559 ]]  @java/lang/Class:exact+120 *, name=instanceCount, idx=6; Volatile! #int !orig=181 !jvms: TestReplaceEquivPhis::test @ bci:39 

If we have a store to the same memory slice as in(5) after the MergeMem node, I think we might trigger one similar bug if we decide to keep in(5) here. 

So I guess it might be safer to go with the initially proposed patch.  

Thanks,
Felix