[15] RFR(S): 8240905: assert(mem == (Node*)1 || mem == mem2) failed: multiple Memories being matched at once?

[Vladimir Kozlov]

On 3/20/20 4:53 AM, Tobias Hartmann wrote:
> Hi,
> 
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8240905
> http://cr.openjdk.java.net/~thartmann/8240905/webrev.00/
> 
> Since JDK-8031321 which added support for Intel's bit manipulation instructions to JDK 8u20, we have
> rules in the .ad file that are matching two LoadNodes and therefore two memory inputs to a single
> instruction (for example, blsiI_rReg_mem [1]). As of today, these BMI1 instructions are still the
> only ones that match multiple memory.

Correction: multiple *uses* of a memory input.

I thought we can't have 2 memory inputs in mach instructions but to my surprise I found 3 cases in x86_32.ad. But they 
are exceptions - they handle FPU instructions: addFPR24_mem_*, mulFPR24_mem_mem.  There are few other in x86_32.ad which 
moves data between memory and stackSlot. But I don't see anything like that in x86_64.ad or other architectures .ad files.

> 
> Since the matcher does not match memory edges, it can happen that the memory inputs of the LoadNodes
> differ and we hit the reported assert. For example, with volatile field accesses, a MemBarAcquire is
> added between the first and the second LoadNode (see test_blsiI_rReg_mem_2). With a product build,
> we would simply ignore the inconsistent memory inputs and match to a 'blsi'. I think this would
> break the happens-before guarantee of volatile memory accesses because the first load is effectively
> re-ordered with the volatile access.
> 
> We already have checks in Matcher::Label_Root() that stop recursion when the memory differs (see [2]
> and [3]) but these only work if the LoadNodes have the same depth in the tree.
> 
> I propose to strengthen these checks and stop recursion if there are multiple loads with different
> memory inputs in the tree. Since that code is using a breadth-first-search, the depth of the first
> load (which will be part of the tree) is always <= the depth of the offending load (which will not
> be part of the tree and is matched into a register). Now I think this could still lead to different
> match results but I couldn't come up with an example where an existing match rule is affected.

I agree with this solution. We will fallback to Register version of instructions for such cases.

And someone have to test 32-bit build with these changes.

Thanks,
Vladimir

> 
> Thoughts?
> 
> Thanks,
> Tobias
> 
> [1] http://hg.openjdk.java.net/jdk/jdk/file/89ec93d09e7e/src/hotspot/cpu/x86/x86_64.ad#l9480
> [2] http://hg.openjdk.java.net/jdk/jdk/file/89ec93d09e7e/src/hotspot/share/opto/matcher.cpp#l1489
> [3] http://hg.openjdk.java.net/jdk/jdk/file/89ec93d09e7e/src/hotspot/share/opto/matcher.cpp#l1525
>