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
