RFR: 8290169: adlc: Improve child constraints for vector unary operations [v2]

[Sandhya Viswanathan]

On Thu, 18 Aug 2022 03:27:55 GMT, Hao Sun <haosun at openjdk.org> wrote:

>> As demonstrated in [1], the child constrait generated for *predicated
>> vector unary operation* is the super set of that generated for the
>> *unpredicated* version. As a result, there exists a risk for predicated
>> vector unary operaions to match the unpredicated rules by accident.
>> 
>> In this patch, we resolve this issue by generating one extra check
>> "rChild == NULL" ONLY for vector unary operations. In this way, the
>> child constraints for predicated/unpredicated vector unary operations
>> are exclusive now.
>> 
>> Following the example in [1], the dfa state generated for AbsVI is shown
>> below.
>> 
>> 
>> void  State::_sub_Op_AbsVI(const Node *n){
>>   if( STATE__VALID_CHILD(_kids[0], VREG) && STATE__VALID_CHILD(_kids[1], PREGGOV) &&
>>       ( UseSVE > 0 ) )
>>   {
>>     unsigned int c = _kids[0]->_cost[VREG]+_kids[1]->_cost[PREGGOV] + SVE_COST;
>>       DFA_PRODUCTION(VREG, vabsI_masked_rule, c)
>>   }
>>   if( STATE__VALID_CHILD(_kids[0], VREG) && _kids[1] == NULL &&  <---- 1
>>       ( UseSVE > 0) )
>>   {
>>     unsigned int c = _kids[0]->_cost[VREG] + SVE_COST;
>>     if (STATE__NOT_YET_VALID(VREG) || _cost[VREG] > c) {
>>       DFA_PRODUCTION(VREG, vabsI_rule, c)
>>     }
>>   }
>>   ...
>> 
>> 
>> We can see that the constraint at line 1 cannot be matched for
>> predicated AbsVI node now.
>> 
>> The main updates are made in adlc/dfa part. Ideally, we should only
>> add the extra check for affected platforms, i.e. AVX-512 and SVE. But we
>> didn't do that because it would be better not to introduce any
>> architecture dependent implementation here.
>> 
>> Besides, workarounds in both ~aarch64_sve.ad~aarch64_vector.ad and x86.ad are removed. 1)
>> Many "is_predicated_vector()" checks can be removed in ~aarch64_sve.ad~aarch64_vector.ad
>> file. 2) Default instruction cost is used for involving rules in x86.ad
>> file.
>> 
>> ~[1]. https://github.com/shqking/jdk/commit/50ec9b19~
>> [1]. https://github.com/shqking/jdk/commit/f7d9621e2
>
> Hao Sun has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into 8290169-adlc
>    
>    Resolve the conflicts.
>  - 8290169: adlc: Improve child constraints for vector unary operations
>    
>    As demonstrated in [1], the child constrait generated for *predicated
>    vector unary operation* is the super set of that generated for the
>    *unpredicated* version. As a result, there exists a risk for predicated
>    vector unary operaions to match the unpredicated rules by accident.
>    
>    In this patch, we resolve this issue by generating one extra check
>    "rChild == NULL" ONLY for vector unary operations. In this way, the
>    child constraints for predicated/unpredicated vector unary operations
>    are exclusive now.
>    
>    Following the example in [1], the dfa state generated for AbsVI is shown
>    below.
>    
>    ```
>    void  State::_sub_Op_AbsVI(const Node *n){
>      if( STATE__VALID_CHILD(_kids[0], VREG) && STATE__VALID_CHILD(_kids[1], PREGGOV) &&
>          ( UseSVE > 0 ) )
>      {
>        unsigned int c = _kids[0]->_cost[VREG]+_kids[1]->_cost[PREGGOV] + SVE_COST;
>          DFA_PRODUCTION(VREG, vabsI_masked_rule, c)
>      }
>      if( STATE__VALID_CHILD(_kids[0], VREG) && _kids[1] == NULL &&  <---- 1
>          ( UseSVE > 0) )
>      {
>        unsigned int c = _kids[0]->_cost[VREG] + SVE_COST;
>        if (STATE__NOT_YET_VALID(VREG) || _cost[VREG] > c) {
>          DFA_PRODUCTION(VREG, vabsI_rule, c)
>        }
>      }
>      ...
>    ```
>    
>    We can see that the constraint at line 1 cannot be matched for
>    predicated AbsVI node now.
>    
>    The main updates are made in adlc/dfa part. Ideally, we should only
>    add the extra check for affected platforms, i.e. AVX-512 and SVE. But we
>    didn't do that because it would be better not to introduce any
>    architecture dependent implementation here.
>    
>    Besides, workarounds in both aarch64_sve.ad and x86.ad are removed. 1)
>    Many "is_predicated_vector()" checks can be removed in aarch64_sve.ad
>    file. 2) Default instruction cost is used for involving rules in x86.ad
>    file.
>    
>    [1]. https://github.com/shqking/jdk/commit/50ec9b19

Looks good to me.

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

Marked as reviewed by sviswanathan (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9534