> ### `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...
Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision: Review ------------- Changes: - all: https://git.openjdk.org/valhalla/pull/2468/files - new: https://git.openjdk.org/valhalla/pull/2468/files/d63f36f3..d1916887 Webrevs: - full: https://webrevs.openjdk.org/?repo=valhalla&pr=2468&range=01 - incr: https://webrevs.openjdk.org/?repo=valhalla&pr=2468&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/valhalla/pull/2468.diff Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2468/head:pull/2468 PR: https://git.openjdk.org/valhalla/pull/2468
