### `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 to be on fire without that. It's not clear to me why it'd be useful, so I removed... ### `TypeAryPtr::empty` https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/type.cpp#L5734-L5737 Here, I went for fixing the underlying `TypeAry::empty`, as fundamentally, that's where the magic happens, and here too, if we have flat and non-flat, we should end-up with an empty type. https://github.com/openjdk/valhalla/blob/f09dea673b68e51ff3604d7c6f0f0561dd0f8c98/src/hotspot/share/opto/type.cpp#L2589-L2591 Thanks to [Add unsigned bounds and known bits to TypeInt/Long](https://bugs.openjdk.org/browse/JDK-8315066), `TypeInt::empty` is constantly false, so `_size->empty()` is redundant. There is the question of how we end-up with something that is both flat and non-flat. If I remember well, it is because of an intersection of a speculative type (that can be flat), and a signature type (assumed non-flat), which must give an empty value. It would be nice if we had bottom-coalescence, that is if one of the component of a product of abstract value is bottom (Hotspot's top), we put everything to bottom (HS's top) since the concretization is empty anyway. That is a simple case of reduction. In our case, it would be nice if, when we see such clash at construction type, to simply go with setting `_elem` to something empty, and not test more than one thing/have a consistent view, whichever part we are looking at. But if we do that in the dual world, we would reduce to bottom (HS's top), something that is the dual of non-empty, and thus, losing information. The easiest fix would simply be to come back later, once we have a proper join, and we get rid of the dual. That will be trivial then. Thanks, Marc --------- - [x] I confirm that I make this contribution in accordance with the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). ------------- Commit messages: - Merge branch 'JDK-8350865.TypeAryPtr_empty' into JDK-8350865 - Trying half a fix - Try to create a crash - Merge branch 'JDK-8350865.MulNode-Value' into JDK-8350865 - MulNode::Value - Merge branch 'JDK-8350865.inline_getFieldMap' into JDK-8350865 - Removed this - Merge branch 'JDK-8350865.inline-isFlat' into JDK-8350865 - No comment - Nevermind - ... and 7 more: https://git.openjdk.org/valhalla/compare/a63fc22f...d63f36f3 Changes: https://git.openjdk.org/valhalla/pull/2468/files Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2468&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8385159 Stats: 118 lines in 7 files changed: 87 ins; 23 del; 8 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
