Re: 答复: [aarch64-port-dev ] RFR(L): 8231441: AArch64: Initial SVE backend support

[Andrew Dinn]

Hi Joshua,

On 10/08/2020 08:24, Joshua Zhu wrote:
> I will help answer questions related with RA.

Thanks for your help.

>>> At line 650 you guard the assert with a test for lrg._is_vector. Is
>>> that not always going to be guaranteed by the outer condition
>>> lrg._is_scalable? If so then you should really assert lrg._is_vector.
> 
> _is_scalable tells the register length for the live range is
> scalable. This rule applies for both SVE vector register and
> predicate register. Each predicate register holds one bit per
> byte of SVE vector register, meaning that each predicate 
> register is one-eighth of the size of SVE vector register.
> Each predicate register is an IMPLEMENTATION DEFINED multiple
> of 16 bits, up to 256 bits. Although the actual length of
> predicate register is scalable, the max slots is always defined
> as 1.> class PRegisterImpl: public AbstractRegisterImpl {
>  public:
>   enum {
>     number_of_registers = 16,
>     max_slots_per_register = 1
>   }; 
> I think this patch under review does not include the part of
>  predicate register allocation.

Ok, I understand that _is_scalable is meant to identify both a predicate
register and an SVE vector register. Something definitely seems to be
missing because field LRG::_is_scalable is not set in the case where we
have a PRegisterImpl (Op_RegVMask). In webrev03 it only ever gets set at
chaitin.cpp:822:

        if (RegMask::is_vector(ireg)) {
          lrg._is_vector = 1;
          if (ireg == Op_VecA) {
            assert(Matcher::supports_scalable_vector(), "scalable vector
should be supported");
            lrg._is_scalable = 1;
            // For scalable vector, when it is allocated in physical
register,
            // num_regs is RegMask::SlotsPerVecA for reg mask,
            // which may not be the actual physical register size.
            // If it is allocated in stack, we need to get the actual
            // physical length of scalable vector register.

lrg.set_scalable_reg_slots(Matcher::scalable_vector_reg_size(T_FLOAT));
          }

So, it seems LRG::_is_scalable will only be set for a VecA register.

If you could check what code might be missing and  post a new webrev
I'll look at this again. However, it would still be good to try to
factor out some common code into methods if possible.

>>> The special case code for computation of num_regs for a vector stack
>>> slot also appears in this file with a slightly different organization
>>> . . .

> PhaseChaitin::Select (line 1590) will cover both SVE vector and predicate cases in future.
> 1590         // We always choose the high bit, then mask the low bits by register size
> 1591         if (lrg->_is_scalable && OptoReg::is_stack(lrg->reg())) { // stack
> 1592           n_regs = lrg->scalable_reg_slots();
> 1593         }
> 
> I think regmask.cpp (line 98) in future will look like:
>  98   if (lrg._is_scalable && OptoReg::is_stack(assigned)) {
>  99     if (lrg._is_vector) {
> 100       assert(ireg == Op_VecA, "scalable vector register");
> 101     }
>         else if (lrg._is_predicate) {
>           assert(ireg == Op_RegVMask, "scalable predicate register");
>         }
> 102     n_regs = lrg.scalable_reg_slots();
> 103   }
> 104 
> 105   return n_regs;
> 106 }
> 
> Please correct me if any issues. Thanks.
Ok, I agree that this will be correct when we can come across the case
where lrg._is_scalable is true and ireg == Op_RegVMask. However, that
case does not currently arise. So, a new webrev that allows for this
case would help.

Thanks for helping to explain what is going on here.

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill