RFR: 8249893: AARCH64: optimize the construction of the value from the bits of the other two [v5]

[Andrew Dinn]

On Wed, 4 Nov 2020 17:40:12 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> Boris Ulasevich has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update comments and function name to make them clearer
>
> src/hotspot/share/opto/addnode.cpp line 872:
> 
>> 870:           return new BitfieldInsertINode(dst, value, phase->intcon(offset), phase->intcon(width));
>> 871:         }
>> 872:       }
> 
> This code and its accompanying comments need to be made much clearer:
> 
>  - `dst` and `src` is a rather perverse choice of names for the inputs of the Or node. `l` and `r` or `left` and `right`,  following the convention in the preceding code, would be better.
> 
>  - You mention `dst` in the comment to identify it as the left `Or` input but do not clearly identify `src` with the two alternative matched patterns for the the right hand side of the `Or`.
> 
>  - The name `shift `is used in those two patterns for an operand that actually has to be a constant bit mask for the transformation to be applicable (so why not use `mask`, `1s_mask` or `constmask`?).
> 
>  - Your comment incoherently employs different notations: i.e. you use a term for the `BitfieldInsert` expression and refer to the outermost node using the term name `Or` but you specify the patterns using the infix C language operators `&` and `<<`.
> 
>  - The qualifiying comment 'if the Or argument value range masks do not overlap' states a condition for the replacement without properly explaining the meaning of that condition i.e. that the various subexpressions operate on disjoint bitfields of the integral value being computed.
> 
>  - You use var `mask` for the const mask node and then reuse it for the constant value it identifies (directly after using `mask` to compute `width` which is used to define `mask`, making it look like mask is used to define itself).
> 
>  - Most importantly, the introductory comment does not provide a clear summary of what sort of graph shape is being replaced and how the match and bit range constraints legitimize that replacement.
> 
> I would suggest the following as a replacement:
> 
>     if (can_reshape && !phase->C->major_progress() && Matcher::match_rule_supported(Op_BitfieldInsertI)) {
>       // If the right input of this Or is an And with mask or an LShifted
>       // And with mask and the left and right inputs can be determined
>       // to construct values lying in disjoint bit ranges then the Or
>       // can be replaced with BitfieldInsert.
>       //
>       // There are two substitution rules:
>       //
>       // 1) (Or left (And value mask)) => (BitfieldInsert left value width 0))
>       //    where width == bitcount(mask) AND
>       //         (value_range_mask(left) & mask) == 0
>       //
>       // 2) (Or left (LShift (And value mask) offset) => (BitfieldInsert left value width 0)
>       //    where width == bitcount(mask) AND
>       //          (value_range_mask(left) & (mask << offset)) == 0
>       // n.b.
>       // mask is an integer constant comprising a contiguous sequence of 1s
>       // value_range_mask(node) computes a mask identifying the range of bits
>       // that could be set by its argument
> 
>       Node *left = in(1);
>       Node *right = in(2);
>       Node *andi = NULL;
>       int offset = 0;
> 
>       if (right->Opcode() == Op_LShiftI && right->in(1)->Opcode() == Op_AndI && right->in(2)->is_Con()) {
>         andi   = right->in(1);
>         offset = right->in(2)->get_int();
>       } else if (right->Opcode() == Op_AndI) {
>         andi = right;
>       }
>       if (andi != NULL) {
>         Node* mask  = andi->in(2);
>         if (mask->is_Con() && is_power_of_2(mask->get_int() + 1)) {
>           Node* value = andi->in(1);
>           int width = exact_log2(mask->get_int() + 1);
>           int maskval = ((1 << width) - 1) << offset;
>           if (width + offset <= 32 && ((value_range_mask(phase, left) & maskval) == 0)) {
>             return new BitfieldInsertINode(left, value, phase->intcon(offset), phase->intcon(width));
>           }
>         }
> 
> Note that I am reusing the SEXPR format used in the ad files to describe the graph patterns and using (C-like) pseudo-code to define the substitution conditions.

Hmm, that is not quite right because in case 2 you are also checking that the shift is constant. So, the overall description should change to say

    // If the right input of this Or is an And with mask or an LShifted
    // And with mask with constant shift and the left and right inputs
    // can be determined . . .

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

PR: https://git.openjdk.java.net/jdk/pull/511