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... This pull request has now been integrated. Changeset: 53268ebc Author: Marc Chevalier <[email protected]> URL: https://git.openjdk.org/valhalla/commit/53268ebc6bf8be67b1c989825cfd70e3046831a9 Stats: 118 lines in 7 files changed: 87 ins; 23 del; 8 mod 8385159: [lworld] C2 parts of JDK-8350865 - Part. 1 Reviewed-by: thartmann ------------- PR: https://git.openjdk.org/valhalla/pull/2468
