RFR: 8305324: C2: Wrong execution of vectorizing Interger.reverseBytes [v4]

[Daohan Qu]

On Wed, 12 Apr 2023 03:48:35 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> I agree. Now that I'm not a hundred per cent sure if number of generated vectors is reduced, I'd better revert some changes. I don't want to make this new function's name misleading (as `Op_LShiftI` doesn't require higher bits info), so I'd rather remove this function. Thanks for your review!
>> 
>> BTW, I notice that you added this if condition at about 2012 in `jdk8u`, do you remember why you test `is_shift` in the if condition instead of something like `is_rshift` or so?
>> 
>>           if (same_type) {
>>             // For right shifts of small integer types (bool, byte, char, short)
>>             // we need precise information about sign-ness. Only Load nodes have
>>             // this information because Store nodes are the same for signed and
>>             // unsigned values. And any arithmetic operation after a load may
>>             // expand a value to signed Int so such right shifts can't be used
>>             // because vector elements do not have upper bits of Int.
>>             const Type* vt = vtn;
>>             if (VectorNode::is_shift(in)) {
>>               Node* load = in->in(1);
>>               if (load->is_Load() && in_bb(load) && (velt_type(load)->basic_type() == T_INT)) {
>>                 vt = velt_type(load);
>>               } else if (in->Opcode() != Op_LShiftI) {
>>                 // Widen type to Int to avoid creation of right shift vector
>>                 // (align + data_size(s1) check in stmts_can_pack() will fail).
>>                 // Note, left shifts work regardless type.
>>                 vt = TypeInt::INT;
>>               }
>>             }
>
>> BTW, I notice that you added this if condition at about 2012 in `jdk8u`, do you remember why you test `is_shift` in the if condition instead of something like `is_rshift` or so?
> 
> At that time we had only 3 shift `Int` vector operations: LShiftI, RShiftI, URShiftI. It did not make sense  to have separate function only for right shift. For loads all operations work since we take type from load. For not loads we left with only RShiftI and URShiftI after excluding LShiftI.
> 
> Note, I am not against executing this code only for right shifts but it needs to be tested. And as separate changes.

Thanks for your detailed explanations! It helps a lot!

BTW, could you sponsor me? :P

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13406#discussion_r1163565515