On Fri, 30 Aug 2024 13:17:26 GMT, Emanuel Peter <[email protected]> wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adding descriptive comments
>
> src/hotspot/cpu/x86/matcher_x86.hpp line 215:
>
>> 213: }
>> 214:
>> 215: static bool vector_indexes_needs_massaging(BasicType ety, int vlen) {
>
> The name "massaging" sounds quite vague. Can we have something more
> expressive / descriptive? Is it the vector that "needs" massaging or the
> indices that "need" massaging?
>
> Why `ety` and not `bt`? Is that not the name we use most often?
Hmm, I see that `ety` is used in other places here. What does it stand for?
> src/hotspot/share/opto/vectornode.cpp line 2183:
>
>> 2181: };
>> 2182: // Targets emulating unsupported permutation for certain vector
>> types
>> 2183: // may need to message the indexes to match the users intent.
>
> Suggestion:
>
> // may need to massage the indexes to match the users intent.
This optimization for now seems quite specific to your
`SelectFromTwoVectorNode::Ideal` lowering code. Can this conversion not be done
there already?
What is the semantics of `VectorRearrangeNode`? Should its shuffle vector
always be bytes, and we now violated that "for a quick second"? Or is it going
to be generally the idea to create all sorts of shuffle types and then fix that
up? But then why do we need the `vector_indexes_needs_massaging`?
Can you help me understand the concept/strategy behind this?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1738714401
PR Review Comment: https://git.openjdk.org/jdk/pull/20508#discussion_r1738862138
