Re: Repost [PATCH 3/6] PowerPC: Add support for accumulators in DMR registers.

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2024/2/7 08:06, Michael Meissner wrote:
> On Thu, Jan 25, 2024 at 05:28:49PM +0800, Kewen.Lin wrote:
>> Hi Mike,
>>
>> on 2024/1/6 07:38, Michael Meissner wrote:
>>> The MMA subsystem added the notion of accumulator registers as an optional
>>> feature of ISA 3.1 (power10).  In ISA 3.1, these accumulators overlapped with
>>> the traditional floating point registers 0..31, but logically the accumulator
>>> registers were separate from the FPR registers.  In ISA 3.1, it was anticipated
>>
>> Using VSX register 0..31 rather than traditional floating point registers 0..31
>> seems more clear, since floating point registers imply 64 bit long registers.
> 
> Ok.
> 
>>> that in future systems, the accumulator registers may no overlap with the FPR
>>> registers.  This patch adds the support for dense math registers as separate
>>> registers.
>>>
>>> This particular patch does not change the MMA support to use the accumulators
>>> within the dense math registers.  This patch just adds the basic support for
>>> having separate DMRs.  The next patch will switch the MMA support to use the
>>> accumulators if -mcpu=future is used.
>>>
>>> For testing purposes, I added an undocumented option '-mdense-math' to enable
>>> or disable the dense math support.
>>
>> Can we avoid this and use one macro for it instead?  As you might have noticed
>> that some previous temporary options like -mpower{8,9}-vector cause ICEs due to
>> some unexpected combination and we are going to neuter them, so let's try our
>> best to avoid it if possible.  I guess one macro TARGET_DENSE_MATH defined by
>> TARGET_FUTURE && TARGET_MMA matches all use places? and specifying -mcpu=future
>> can enable it while -mcpu=power10 can disable it.
> 
> That depends on whether there will be other things added in the future power
> that are not in the MMA+ instruction set.
> 
> But I can switch to defining TARGET_DENSE_MATH to testing TARGET_FUTURE and
> TARGET_MMA.  That way if/when a new cpu comes out, we will just have to change
> the definition of TARGET_DENSE_MATH and not all of the uses.

Yes, that's what I expected.  Thanks!

> 
> I will also add TARGET_MMA_NO_DENSE_MATH to handle the existing MMA code for
> assemble and disassemble when we don't have dense math instructions.

Nice, I also found having such macro can help when reviewing one latter patch
so suggested a similar there.

>>> -(define_insn_and_split "*movxo"
>>> +(define_insn_and_split "*movxo_nodm"
>>>    [(set (match_operand:XO 0 "nonimmediate_operand" "=d,ZwO,d")
>>>  	(match_operand:XO 1 "input_operand" "ZwO,d,d"))]
>>> -  "TARGET_MMA
>>> +  "TARGET_MMA && !TARGET_DENSE_MATH
>>>     && (gpc_reg_operand (operands[0], XOmode)
>>>         || gpc_reg_operand (operands[1], XOmode))"
>>>    "@
>>> @@ -366,6 +369,31 @@ (define_insn_and_split "*movxo"
>>>     (set_attr "length" "*,*,16")
>>>     (set_attr "max_prefixed_insns" "2,2,*")])
>>>  
>>> +(define_insn_and_split "*movxo_dm"
>>> +  [(set (match_operand:XO 0 "nonimmediate_operand" "=wa,QwO,wa,wD,wD,wa")
>>> +	(match_operand:XO 1 "input_operand"        "QwO,wa, wa,wa,wD,wD"))]
>>
>> Why not adopt ZwO rather than QwO?
> 
> You have to split the address into 2 addresses for loading or storing vector
> pairs (or 4 addresses for loading or storing vectors).  Z would allow
> register+register addresses, and you wouldn't be able to create the second 
> address by adding 128 to it.  Hence it uses 'Q' for register only and 'wo' for
> d-form addresses.

Thanks for clarifying.  But without this patch the define_insn_and_split *movxo
adopts "ZwO", IMHO it would mean the current "*movxo" define_insn_and_split have
been problematic?  I thought adjust_address can ensure the new address would be
still valid after adjusting 128 offset, could you double check?

> 
>>
>>> +  "TARGET_DENSE_MATH
>>> +   && (gpc_reg_operand (operands[0], XOmode)
>>> +       || gpc_reg_operand (operands[1], XOmode))"
>>> +  "@
>>> +   #
>>> +   #
>>> +   #
>>> +   dmxxinstdmr512 %0,%1,%Y1,0
>>> +   dmmr %0,%1
>>> +   dmxxextfdmr512 %0,%Y0,%1,0"
>>> +  "&& reload_completed
>>> +   && !dmr_operand (operands[0], XOmode)
>>> +   && !dmr_operand (operands[1], XOmode)"
>>> +  [(const_int 0)]
>>> +{
>>> +  rs6000_split_multireg_move (operands[0], operands[1]);
>>> +  DONE;
>>> +}
>>> +  [(set_attr "type" "vecload,vecstore,veclogical,mma,mma,mma")
>>> +   (set_attr "length" "*,*,16,*,*,*")
>>> +   (set_attr "max_prefixed_insns" "2,2,*,*,*,*")])
>>> +

...

>>> +;; Return 1 if op is a DMR register
>>> +(define_predicate "dmr_operand"
>>> +  (match_operand 0 "register_operand")
>>> +{
>>> +  if (!REG_P (op))
>>> +    return 0;
>>> +
>>> +  if (!HARD_REGISTER_P (op))
>>> +    return 1;
>>> +
>>> +  return DMR_REGNO_P (REGNO (op));
>>> +})
>>> +
>>> +;; Return 1 if op is an accumulator.  On power10 systems, the accumulators
>>> +;; overlap with the FPRs, while on systems with dense math, the accumulators
>>> +;; are separate dense math registers and do not overlap with the FPR
>>> +;; registers..
>>
>> Nit: an unexpected "."?
>>
>>> +(define_predicate "accumulator_operand"
>>> +  (match_operand 0 "register_operand")
>>> +{
>>
>> fpr_reg_operand checks for subreg as well, should we check for it here as well?
>>
>>>  #ifdef TARGET_REGNAMES
>>> @@ -1250,6 +1255,8 @@ static const char alt_reg_names[][8] =
>>>    "%cr0",  "%cr1", "%cr2", "%cr3", "%cr4", "%cr5", "%cr6", "%cr7",
>>>    /* vrsave vscr sfp */
>>>    "vrsave", "vscr", "sfp",
>>> +  /* DMRs */
>>> +  "%dmr0", "%dmr1", "%dmr2", "%dmr3", "%dmr4", "%dmr5", "%dmr6", "%dmr7",
>>
>> Should be without "r" here, as tested gas doesn't recognize %dmr0 but it does
>> recognize %dm0.

I guessed some reply was missing on this part (and some latter others)?  Just want
to ensure something wasn't missing and hope this helps.  :)

>>
>>>  };
>>>  #endif
>>>  
>>> @@ -1846,6 +1853,9 @@ rs6000_hard_regno_nregs_internal (int regno, machine_mode mode)
>>>    else if (ALTIVEC_REGNO_P (regno))
>>>      reg_size = UNITS_PER_ALTIVEC_WORD;
>>>  
>>> +  else if (DMR_REGNO_P (regno))
>>> +    reg_size = UNITS_PER_DMR_WORD;
>>> +
>>>    else
>>>      reg_size = UNITS_PER_WORD;
>>>  
>>> @@ -1867,9 +1877,36 @@ rs6000_hard_regno_mode_ok_uncached (int regno, machine_mode mode)
>>>    if (mode == OOmode)
>>>      return (TARGET_MMA && VSX_REGNO_P (regno) && (regno & 1) == 0);
>>>  
>>> -  /* MMA accumulator modes need FPR registers divisible by 4.  */
>>> +  /* On ISA 3.1 (power10), MMA accumulator modes need FPR registers divisible
>>> +     by 4.
>>> +
>>> +     If dense math is enabled, allow all VSX registers plus the DMR registers.
>>> +     We need to make sure we don't cross between the boundary of FPRs and
>>> +     traditional Altiviec registers.  */
>>>    if (mode == XOmode)
>>> -    return (TARGET_MMA && FP_REGNO_P (regno) && (regno & 3) == 0);
>>> +    {
>>> +      if (TARGET_MMA && !TARGET_DENSE_MATH)
>>> +	return (FP_REGNO_P (regno) && (regno & 3) == 0);
>>> +
>>> +      else if (TARGET_DENSE_MATH)
>>> +	{
>>> +	  if (DMR_REGNO_P (regno))
>>> +	    return 1;
>>> +
>>> +	  if (FP_REGNO_P (regno))
>>> +	    return ((regno & 1) == 0 && regno <= LAST_FPR_REGNO - 3);
>>> +
>>> +	  if (ALTIVEC_REGNO_P (regno))
>>> +	    return ((regno & 1) == 0 && regno <= LAST_ALTIVEC_REGNO - 3);
>>> +	}
>>
>> I could miss something, I didn't find which section of RFC indicates this
>> restriction, could you please point out for me?  Thanks!
>>
>>> +
>>> +      else
>>> +	return 0;
>>> +    }
>>> +
>>> +  /* No other types other than XOmode can go in DMRs.  */
>>> +  if (DMR_REGNO_P (regno))
>>> +    return 0;
>>>  
>>>    /* PTImode can only go in GPRs.  Quad word memory operations require even/odd
>>>       register combinations, and use PTImode where we need to deal with quad
>>> @@ -2312,6 +2349,7 @@ rs6000_debug_reg_global (void)
>>>    rs6000_debug_reg_print (FIRST_ALTIVEC_REGNO,
>>>  			  LAST_ALTIVEC_REGNO,
>>>  			  "vs");
>>> +  rs6000_debug_reg_print (FIRST_DMR_REGNO, LAST_DMR_REGNO, "dmr");
>>
>> Nit: Like above, use 'dm'.
>>
>>>    rs6000_debug_reg_print (LR_REGNO, LR_REGNO, "lr");
>>>    rs6000_debug_reg_print (CTR_REGNO, CTR_REGNO, "ctr");
>>>    rs6000_debug_reg_print (CR0_REGNO, CR7_REGNO, "cr");
>>> @@ -2332,6 +2370,7 @@ rs6000_debug_reg_global (void)
>>>  	   "wr reg_class = %s\n"
>>>  	   "wx reg_class = %s\n"
>>>  	   "wA reg_class = %s\n"
>>> +	   "wD reg_class = %s\n"
>>>  	   "\n",
>>>  	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
>>>  	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
>>> @@ -2339,7 +2378,8 @@ rs6000_debug_reg_global (void)
>>>  	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_we]],
>>>  	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wr]],
>>>  	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wx]],
>>> -	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wA]]);
>>> +	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wA]],
>>> +	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wD]]);
>>>
>>
>> snip ...
>>
>>> +/* Subroutine to determine the move cost of dense math registers.  If we are
>>> +   moving to/from VSX_REGISTER registers, the cost is either 1 move (for
>>> +   512-bit accumulators) or 2 moves (for 1,024 dmr registers).  If we are
>>> +   moving to anything else like GPR registers, make the cost very high.  */
>>> +
>>> +static int
>>> +rs6000_dmr_register_move_cost (machine_mode mode, reg_class_t rclass)
>>> +{
>>> +  const int reg_move_base = 2;
>>> +  HARD_REG_SET vsx_set = (reg_class_contents[rclass]
>>> +			  & reg_class_contents[VSX_REGS]);
>>> +
>>> +  if (TARGET_DENSE_MATH && !hard_reg_set_empty_p (vsx_set))
>>
>> Can we just use reg_classes_intersect_p (rclass, VSX_REGS)?
>>


BR,
Kewen