On Mon, 8 Jun 2026 07:07:13 GMT, Emanuel Peter <[email protected]> wrote:
>> Hi @XiaohongGong , >> >> Take inline_vector_compare as an example. The lane type passed from the Java >> side is LT_FLOAT16, but match_rule_supported_vector operates on the vector >> IR opcode, the basic type (which for Float16 is short), and num_elem. So a >> Float16 compare would be matched as Op_VectorMaskCmp with elem_bt=short and >> collide with the existing short-integer VectorMaskCmpNode used by >> ShortVector — giving wrong FP semantics. >> >> We would need a distinct Float16 compare IR (as we already did for the other >> supported vector Float16 ops) before this can be guarded purely by >> match_rule_supported_vector. That is why, for now, suppressing >> intrinsification based on lane type is the clean approach; in subsequent >> patches we can drop the is_unsupported_lane_type checks one by one and rely >> on match_rule_supported_vector with the new opcodes, which will also guard >> platforms lacking support. >> >> This PR mainly adds the Java-side support and enables intrinsification only >> for operations that already have auto-vectorization support. I hope this >> looks reasonable. > > Thanks for the explanations! Sounds like we now might have a bit of a design > mess with float16/short confusion in the VM. I hope we can get to a clean > design in the future. > > But about `is_unsupported_lane_type` specifically: my concern is that it is > not good design. Now you might argue it is transitional code only, and so we > should be ok with bad design here. But it often happens that transitional > code stays for a while, and it would be quite easy to flip this to better > design. > > My opinion: > **Don't exclude bad things. Only allow good things.** > Otherwise, you'll at some point forget another bad thing, and forget to > exclude it, and we get incorrect code. > That's worse than forgetting a good thing: we just get slower but still > correct code. > > So I'd ask you to flip it to a `is_supported_lane_type`, and then just accept > only primitive types for now, and later also allow float16, once we make > those extensions. I see, thanks! I was thinking there is a new BasicType for FP16 added as well. I do not suggest adding new IRs for FP16 types. Do you think it's better to maintain a list of supported vector ops for FP16 instead of checking the type for each intrinsic? We can check the list firstly in `arch_supports_vector`, and remove the list once all the vector ops are supported in future. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28002#discussion_r3371394821
