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

[Daohan Qu]

On Tue, 11 Apr 2023 17:52:26 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Thanks for your review. Yes, I see. AFAICS, the if-condition calling this function wants to check whether higher order bits are needed. So I distill the condition content into a function. The `Op_LShiftI` is excluded since it doesn't need such info. Do I miss something? Or should the if-condition have checked more than what I thought?
>
> What I am trying to say is that before this change `vt` will be set to `velt_type(load)` even if `in` is `LShiftI` node. With your changes `vt` will stay `== vtn` if  `in` is `LShiftI` node.  `velt_type(load)` could be different from `vtn` and as result your change may introduce difference in code generation in other than `ReverseBytesI` cases.
> This needs to be tested to see if number of generated vectors is not reduced for such cases.

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;
              }
            }

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

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