On Fri, 22 May 2026 08:58:48 GMT, Marc Chevalier <[email protected]> wrote:
> ### `LibraryCallKit::inline_getArrayProperties` > https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/library_call.cpp#L4805-L4807 > > Done. > > This one uncovered a bug, then I ran into something unrelated, but polluting > testing. It looks good now. The logic is not quite simple, but it is exactly > as it is described in the runtime version of it. > > ### `LibraryCallKit::inline_getArrayProperties` > https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/library_call.cpp#L4797-L4798 > > This one, we need to keep. `inline_getArrayProperties` and `flat_array_test` > builds a graph like > > CMoveI(Bool(FlatArrayCheck(mem, array_klass), cmp), 0, 1, BOOL) > > which is fine for > https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/macro.cpp#L3047-L3049 > and we take the else-branch. But if instead of `array_klass`, we had just an > `array`, we would take the if-branch. And this one expects a different shape: > - it looks for the `BoolNode` (let it be `bol`) under the `FlatArrayCheckNode` > - it looks at outputs of `bol`, and for each, expects it to be an `IfNode` > - use the control of the `IfNode` to build more control flow (with the > lock/unlock paths...) > > Clearly, our graph has a `CMoveINode` under the `BoolNode`, so this crashes. > It's not just a matter of `expand_flatarraycheck_node` being too strict, but > rather the problem is that we need a control input for this expansion, and we > don't have with the `CMoveINode`. > > I can see two easy solutions: > - change `inline_getArrayProperties` to generate something more like > > Phi( > Region( > r, > IfTrue(If(ctrl, Bool(FlatArrayCheck(mem, array_klass), cmp)) as iff), > IfFalse(iff) > ) as r, > 0, > 1 > ) > > which would allow the expansion to work. > - not change anything. > > I went with not changing anything because loading the class seems less bad > (and might already happen, and thus be share, as it is used by some other > operations) than making a more complicate structure, that is control-pinned. > > > > ### `LibraryCallKit::inline_getFieldMap` > https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/library_call.cpp#L3186-L3187 > > Yes, sir! Testing seems to be happy. > > ### `MulNode::Value` > https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/mulnode.cpp#L202-L213 > > There is no test that seems... Thanks for working on this Marc! Looks good to me. src/hotspot/share/opto/type.cpp line 2591: > 2589: bool TypeAry::empty(void) const { > 2590: assert(!_size->empty(), "TypeInt is never empty"); > 2591: // TODO 8370914 This should be simplified at construction time once > we get rid of dual We might miss this TODO when fixing JDK-8370914, so maybe it's better to create a separate (Valhalla) bug for this. Also, I think this task is out of scope of JDK-8370914. ------------- Marked as reviewed by thartmann (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2468#pullrequestreview-4361169579 PR Review Comment: https://git.openjdk.org/valhalla/pull/2468#discussion_r3301691122
