On Wed, 3 Jun 2026 13:55:44 GMT, Roland Westrelin <[email protected]> wrote:
>> The bug is caused by a mismatch in the way `LoadFlat` and `StoreFlat` >> are handled in `ConnectionGraph::find_inst_mem()` and >> `MemNode::optimize_simple_memory_chain()`. Once EA has run a first >> time, found some value array allocations as non escaping, and removed >> their `LoadFlat`/`StoreFlat`, igvn runs and is able to further >> transform the memory subgraph which introduces inconsistencies that >> are then caught the next time EA runs. >> >> The fix tries to make sure `ConnectionGraph::find_inst_mem()` and >> `MemNode::optimize_simple_memory_chain()` handle `LoadFlat` and >> `StoreFlat` similarly. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Roland Westrelin has updated the pull request incrementally with one > additional commit since the last revision: > > review Few comments/questions. src/hotspot/share/opto/escape.cpp line 4536: > 4534: #define FIND_INST_MEM_RECURSION_DEPTH_LIMIT 1000 > 4535: > 4536: bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const > TypeOopPtr *toop) { Add comment about what this method does. src/hotspot/share/opto/escape.cpp line 4537: > 4535: > 4536: bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const > TypeOopPtr *toop) { > 4537: Node* base = flat_access->is_StoreFlat() ? > flat_access->as_StoreFlat()->base() : flat_access->as_LoadFlat()->base(); What type of `base` node could be here? src/hotspot/share/opto/escape.cpp line 4538: > 4536: bool ConnectionGraph::flat_access_aliases_with(Node* flat_access, const > TypeOopPtr *toop) { > 4537: Node* base = flat_access->is_StoreFlat() ? > flat_access->as_StoreFlat()->base() : flat_access->as_LoadFlat()->base(); > 4538: const TypeOopPtr* t_base = _igvn->type(base)->is_oopptr(); This statement is too early - far before uses. src/hotspot/share/opto/escape.cpp line 4550: > 4548: if (es >= PointsToNode::GlobalEscape) { > 4549: return false; > 4550: } Why not check here? Asserts will not be needed then. const TypeOopPtr* t_base = _igvn->type(base)->is_oopptr(); if (t_base->instance_id() == toop->instance_id()) { return true; } src/hotspot/share/opto/escape.cpp line 4651: > 4649: // ConnectionGraph::optimize_flat_accesses() and has no effect > on the memory state > 4650: // - or it is a LoadFlat for some object unrelated to alias_idx > 4651: // In both cases, it's safe to assume this LoadFlat doesn't > modify the memory for alias_idx Which flat load can modify memory? I surprise that this node has Memory projection. src/hotspot/share/opto/escape.cpp line 4654: > 4652: // If the LoadFlat is mismatched, it's not removed so don't > step over it if it's a flat access to the alias_idx > 4653: // known instance > 4654: if (!proj_in->as_LoadFlat()->is_mismatched() || > !flat_access_aliases_with(proj_in, toop)) { When it can be `mismatched`? Your whole comment above just confuse me. What I see is in normal case (not mismatched) we always step over loads regardless memory slice it is on. In mismatching case we skip if it is on different memory slice. That is it. ------------- PR Review: https://git.openjdk.org/valhalla/pull/2489#pullrequestreview-4420125338 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3349830382 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3350051263 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3350257004 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3350254180 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3350120401 PR Review Comment: https://git.openjdk.org/valhalla/pull/2489#discussion_r3350200116
