On 6/18/20 6:44 PM, Segher Boessenkool wrote:
>> (rs6000_builtin_mask_calculate): Add support for RS6000_BTM_MMA
>> and RS6000_BTM_FUTURE.
> The latter is already there?
Oops, yes. I'll remove it.
>> * config/rs6000/rs6000.md (define_attr "isa"): Add mma.
>
> Is this ever useful? Please leave it out if not. The "isa" things
> are only for when some insn alternatives are available only one some
> configurations and not others (not for when the whole pattern is not
> valid).
I think I added it back when we had a "pair" isa attribute and I
think I thought I needed it then. I think you are correct that
we don't need it now. I'll remove it.
>> (define_mode_iterator RELOAD): Add POI and PXI.
>
> Why POI and PXI, but not OI and XI?
We don't have an enabled XI or OI move pattern, so I don't think
we'll ever see those modes at all in rtl.
>> Include mma.md.
>
> That looks to be about RELOAD, the way it is placed. Maybe put it as
> the very first thing for this file, in the changelog?
Yo mean rewrite it like the following?
...
* config/rs6000/rs6000.md: Include mma.md.
(define_attr "isa"): Add mma.
(define_attr "enabled"): Handle mma.
(define_mode_iterator RELOAD): Add POI and PXI.
...
...or do you mean move the rs6000.md entry to be the first entry in the
ChangeLog?
>> +;; cause byte swapping issues on litte-endian systems. We don't need
>> +;; the XImode and OImode move patterns for actual code generation,
>> +;; therefor, we define the XImode and OImode move patterns, but we
>> +;; disable their use with a "false" condition flag.
>
> "therefore".
Fixed.
>> +;; Define a disabled OImode move pattern, so we can use POImode.
>> +(define_expand "movoi"
>> + [(set (match_operand:OI 0 "nonimmediate_operand")
>> + (match_operand:OI 1 "input_operand"))]
>> + "0"
>> +{
>> + gcc_unreachable ();
>> +})
>
> So dirty, I love it :-)
Heh, credit to Mike on this one.
>> +(define_insn_and_split "*movpoi"
>> + [(set (match_operand:POI 0 "nonimmediate_operand" "=wa,m,wa")
>> + (match_operand:POI 1 "input_operand" "m,wa,wa"))]
>
> Don't use tabs other than at the start of the line, please (or *maybe*
> in tables).
Fixed. I just replaced it with one space to match the *movpxi pattern.
>> +;; Special pattern to prevent DSE from generating an internal error if it
>> +;; notices a structure copy that it wants to eliminate. This generates
>> pretty
>> +;; bad code, but at least it doesn't die.
>> +(define_insn_and_split "truncpoidi2"
>
> Could you say *why*/*how* it prevents the ICE here?
This was added by Mike. I didn't debug the issue. Mike, do you have
some verbiage we could add here?
>> + [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
>> + (truncate:DI (match_operand:POI 1 "gpc_reg_operand" "wa")))]
>> + "TARGET_MMA"
>> + "#"
>> + "&& reload_completed"
>> + [(set (match_dup 0)
>> + (vec_select:DI (match_dup 2)
>> + (parallel [(match_dup 3)])))]
>> +{
>> + unsigned r = reg_or_subregno (operands[1]) + !BYTES_BIG_ENDIAN;
>
> Don't + booleans please (use ?: instead).
>
>> + operands[2] = gen_rtx_REG (V2DImode, r);
>> + operands[3] = BYTES_BIG_ENDIAN ? const1_rtx : const0_rtx;
>> +})
>
> So maybe just do an if (BYTES_BIG_ENDIAN) even, the arms simplify a
> bit then.
Like so?
if (BYTES_BIG_ENDIAN)
{
operands[2] = gen_rtx_REG (V2DImode, reg_or_subregno (operands[1]));
operands[3] = const1_rtx;
}
else
{
operands[2] = gen_rtx_REG (V2DImode, reg_or_subregno (operands[1]) + 1);
operands[3] = const0_rtx;
}
>> +;; Vector quad support. PXImode is only defined for floating point
>> registers.
>
> Rephrase this? A mode is defined without referring to registers at
> all ;-) "PXImode can only live in FPRs", something like that?
Ok, I changed it to that. I assume you want the same thing changed for POImode
too, so I modified its comment to "POImode can only live in VSRs.".
>> + /* Vector pair modes need even/odd VSX register pairs. Only allow vector
>> + registers. We need to allow OImode to have the same registers as
>> POImode,
>> + even though we do not enable the move pattern for OImode. */
>> + if (mode == POImode || mode == OImode)
>> + return (TARGET_MMA && VSX_REGNO_P (regno)
>> + && (regno & 1) == 0);
>
> Put it all one one line?
>
>> + /* MMA accumulator modes need FPR registers divisible by 4. We need to
>> allow
>> + XImode to have the same registers as PXImode, even though we do not
>> enable
>> + the move pattern for XImode. */
>> + if (mode == PXImode || mode == XImode)
>> + return (TARGET_MMA && FP_REGNO_P (regno)
>> + && (regno & 3) == 0);
>
> Likewise.
Done.
> Why are OImode and XImode handled here?
>
>> static bool
>> rs6000_modes_tieable_p (machine_mode mode1, machine_mode mode2)
>> {
Do you mean why *aren't* they handled in rs6000_modes_tieable_p?
Probably because since we don't generate them, we didn't think we
need to handle them. Do you want me to add them?
>> - if (mode1 == PTImode)
>> - return mode2 == PTImode;
>> - if (mode2 == PTImode)
>> + if (mode1 == PTImode || mode1 == POImode || mode1 == PXImode)
>> + return mode1 == mode2;
>> + if (mode2 == PTImode || mode2 == POImode || mode2 == PXImode)
>> return false;
>
> You can just do
> if (mode1 == PTImode || mode1 == POImode || mode1 == PXImode
> || mode2 == PTImode || mode2 == POImode || mode2 == PXImode)
> return mode1 == mode2;
Ok, changed. Let me know if you want me to also add OImode and XImode
there too.
>> @@ -2206,6 +2227,8 @@ rs6000_debug_reg_global (void)
>> SDmode,
>> DDmode,
>> TDmode,
>> + V2SImode,
>> + V2SFmode,
>
> Did the changelog mention these? If it is a bugfix, could it need a
> backport? Do it a separate patch then?
>
> Well, it is debug info only, so not really interesting, but heh.
>
>> @@ -2220,9 +2243,14 @@ rs6000_debug_reg_global (void)
>> V2DFmode,
>> V8SFmode,
>> V4DFmode,
>> + OImode,
>> + XImode,
>> + POImode,
>> + PXImode,
>> CCmode,
>> CCUNSmode,
>> CCEQmode,
>> + CCFPmode,
>> };
>
> Same for the CCFP one here.
Mike added those. I guess I thought they were needed. Mike?
If they're not needed for MMA, I'll remove them from this patch
and they be submitted in a separate patch if they are needed for
something else.
>> + /* Add support for vector pairs and vector quad registers. */
>> + if (TARGET_MMA)
>> + {
>> + for (m = 0; m < NUM_MACHINE_MODES; ++m)
>> + if (m == POImode || m == PXImode)
>> + {
>> + rs6000_vector_unit[m] = VECTOR_NONE;
>> + rs6000_vector_mem[m] = VECTOR_VSX;
>> + rs6000_vector_align[m] = (m == POImode) ? 256 : 512;
>> + }
>> + }
>
> This is just
>
> /* Add support for vector pairs and vector quad registers. */
> if (TARGET_MMA)
> {
> rs6000_vector_unit[POImode] = VECTOR_NONE;
> rs6000_vector_mem[POImode] = VECTOR_VSX;
> rs6000_vector_align[POImode] = 256;
>
> rs6000_vector_unit[PXImode] = VECTOR_NONE;
> rs6000_vector_mem[PXImode] = VECTOR_VSX;
> rs6000_vector_align[PXImode] = 512;
> }
>
> which is just not longer (even although is has that whiteline :-) )
Heh, yes duh! Changed.
>> + reg_mode = POImode;;
>> + nregs /= hard_regno_nregs (reg, reg_mode);
>> + }
>
> (doubled semicolon)
Fixed.
> So nregs is always 2? Maybe it is better to just assert that here then?
If in a VSR, yes. I think maybe we thought early on that if they were somehow
in a GPR then they'd take more regs, but I think maybe we've guarantee that
can't happen??? I can set it to 2 and add an assert and see if that exposes
anything.
>> @@ -19249,6 +19416,9 @@ rs6000_mangle_type (const_tree type)
>> if (SCALAR_FLOAT_TYPE_P (type) && FLOAT128_IEEE_P (TYPE_MODE (type)))
>> return ieee128_mangling_gcc_8_1 ? "U10__float128" : "u9__ieee128";
>>
>> + if (type == vector_pair_type_node) return "u13__vector_pair";
>> + if (type == vector_quad_type_node) return "u13__vector_quad";
>
> Line breaks?
Ok, added line breaks. I was kind of just following some of the code
before it. Of course other code there does it the other way! :-)
>> /* No data type wants to be aligned rounder than this. */
>> -#define BIGGEST_ALIGNMENT 128
>> +#define BIGGEST_ALIGNMENT ((TARGET_MMA) ? 512 : 128)
>
> No silly parens around TARGET_MMA please (macros should protect
> themselves, sure, but not try to protect other macros).
Fixed.
> Okay for trunk modulo the above. Thanks! This was much less painful
> than I feared. Well, maybe it is all in the other patches, I'll get to
> those tomorrow ;-)
Thanks!
Mike, can you answer the 2 questions for you above?
Peter
