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

[Joshua Zhu]

Hi Andrew,

Thanks a lot for your review.

> As Ningsheng is on leave this week, I will attempt to answer your specific
> questions based on what I know. I'm sorry that I'm not able to answer all
> your questions since I'm not familiar with every detail of the patch. And you
> may still need to wait him coming back to update the webrev(s).

I will help answer questions related with RA.

> > Specific Comments (register allocator webrev):
> >
> >
> > aarch64.ad:97-100
> >
> > Why have you added a reg_def for R8 and R9 here and also to
> > alloc_class
> > chunk0 at lines 544-545? They aren't used by C2 so why define them?
> 
> This has no functionality change to the two scratch registers. But if these are
> missing in the register definition, the regmask for vector registers won't start
> at an aligned position. So we prefer adding them back to make the
> computation easier.

Yes. Thanks Pengfei.

> 
> > assembler_aarch64.hpp:280 (also 699)
> >
> > prf sets a predicate register field. pgrf sets a governing predicate register
> field.
> > Should the name not be gprf.
> 
> I guess the reason is that the ArmARM doc says "the Pg field".
> 
> > chaitin.cpp:648-660
> >
> > The comment is rather oddly formatted.
> 
> Thanks for catching this.
> 
> > 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.

> > The special case code for computation of num_regs for a vector stack
> > slot also appears in this file with a slightly different organization
> > in find_first_set (line 1350) and in PhaseChaitin::Select (line 1590).
> > There is another similar case in RegMask::num_registers at regmask.cpp:
> > 98. It would be better to factor out the common code into methods of LRG.
> > Maybe using the following?
> >
> >   bool LRG::is_scalable_vector() {
> >     if (_is_scalable) {
> >       assert(_is_vector == 1);
> >       assert(_num_regs == == RegMask::SlotsPerVecA)
> >       return true;
> >     }
> >     return false;
> >   }
> >
> >   int LRG::scalable_num_regs() {
> >     assert(is_scalable_vector());
> >     if (OptoReg::is_stack(_reg)) {
> >       return _scalable_reg_slots
> >     } else {
> >       return num_reg_slots;
> >     }
> >   }
> >
> > chaitin.cpp:1350
> >
> > Once again the test for lrg._is_vector should be guaranteed by the
> > outer test of lrg._is_scalable. Refactoring using the common methods
> > of LRG as above ought to help.
> >
> > chaitin.cpp:1591
> >
> > Use common method code.
> >
> >
> > postaloc.cpp:308/323
> >
> > Once again you should be able to use common method code of LRG here.
> >
> >
> > regmask.cpp:91
> >
> > Once again you should be able to use common method code of LRG here.

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.

Best Regards,
Joshua