On Wed, 20 May 2026 07:54:50 GMT, Aleksey Shipilev <[email protected]> wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Accepting more suggestions from Andrew Dinn.
>
> src/hotspot/cpu/aarch64/register_aarch64.hpp line 546:
>
>> 544: template<int N>
>> 545: VSeq<N-1> vs_tail(const VSeq<N>& v) {
>> 546: static_assert(N > 1, "tail sequence length must be greater than 2");
>
> Which one is it, `N > 1`, or `greater than 2`?
Full disclosure -- this is my copypasta introduced during an earlier round of
reviewing.
Given that we have a static assert that `N > 2` in the definition of class
`VSeq` I guess this is redundant. Likewise, the assertion in `vs_head`.
It doesn't really make any sense to /declare/ a `VSeq` with less than 2
elements as the purpose of the class is to provide a way to manage and access
more than one FloatRegister using indexed notation (`vs[i]`). However, the
provision of `vs_tail` gives rise to the possibility that we might create a
`VSeq<1>` from a `VSeq<2>` so we could consider adjusting the asserts
accordingly.
In fact, `vs_head` and `vs_tail` were added to deal with the need for the
current proposed kernel to operate on groups of 5 registers at a time. The
code uses `vs_head` to access the first register in the group and `vs_tail` to
access a sequence over the remaining four registers (n.b. calling `vs_head(vs)`
is not strictly needed -- you can just use `vs[0]` -- so maybe we don't need
to provide it).
So, for current requirements, we could just delete the assert here and in
`vs_head` and rely on the one in the definition of `VSeq`. If we ever need to
split off the tail of a `VSeq<2>` we could reconsider this.
Likewise we could drop `vs_head`. I thought it would be good to provide it
because it emphasise the way that the sequence is being recursively split in
two and ... well ... Lisp!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30941#discussion_r3274978618
