> -----Original Message----- > From: Segher Boessenkool <[email protected]> > Sent: 12 October 2025 20:03 > To: Tamar Christina <[email protected]> > Cc: [email protected]; nd <[email protected]>; [email protected]; > [email protected]; [email protected] > Subject: Re: [PATCH 6/9][rs6000]: convert widen_[us]sum into convert optab > [PR122069] > > Hi! > > On Sun, Oct 12, 2025 at 05:00:59PM +0000, Tamar Christina wrote: > > > -----Original Message----- > > > From: Segher Boessenkool <[email protected]> > > > Sent: 12 October 2025 16:58 > > > To: Tamar Christina <[email protected]> > > > Cc: [email protected]; nd <[email protected]>; [email protected]; > > > [email protected]; [email protected] > > > Subject: Re: [PATCH 6/9][rs6000]: convert widen_[us]sum into convert > optab > > > [PR122069] > > > > > > On Fri, Oct 03, 2025 at 10:46:34AM +0100, Tamar Christina wrote: > > > > This patch is a mechanical rewrite of the widen_[us]sum optabs from a > direct > > > to > > > > a conversion optab. > > > > > > There is no optab (or table of any other kind) in this patch. Instead, > > > it changes some pattern names, from well-known sort-of-standard names > to > > > not-so-standard names. > > > > > > Could you please make these new names standard (i.e. DOCUMENTED) > first? > > > And then refer to that documentation in this patch :-) > > > > They are documented, they are changed in patch 1. [1] > > > > [1] https://patchwork.ozlabs.org/project/gcc/patch/patch-19869- > [email protected]/ > > > > This patch is blocking the documentation update commit :) > > You have all sequencing reversed, then. Docs come first, always. > > > > > (widen_usumv4si<mode>3): ... into this. > > > > > > Those new names are incorrect in any case, it should be > > > widen_usumv4si<mode>2! There are only *two* <mode> thing operands > in > > > the pattern, after all. That is what the number means: something like > > > "addsi3" is sort of shorthand for "addsisisi" (and addsi2di means > > > addsisidi). > > > > The number just indicates the number of operands, there are 3, so the > number > > is correct, that's the convention established by the middle-end > > and that's what the patch follows. Before this there was only 1 mode and it > was still 3. > > Some people use that convention. And some do not. That latter > convention has older history (and makes way more sense). How can your > proposed naming distinguish between an add with a SIMode and two DImode > operands, and one with two SImode and onbe DImode ops? It cannot. >
It's restricted by the number of modes the optab declaration has mentioned
matters for selection. The operation *requires* that operand 0 and 2 be of
the same mode and be wider than operand 1. This is checked during expansion.
The optabs are declared as
OPTAB_CD(ssum_widen_optab, "widen_ssum$a$b3")
OPTAB_CD(usum_widen_optab, "widen_usum$a$b3")
So they are explicitly saying that two modes uniquely identify the operation.
And so since it is required that the output and one input mode be the same
the case of " and one with two SImode and onbe DImode ops" is simply not a valid
widening sum combination and would never be used.
The wider mode is required to be the output, and it's required there be two
such modes.
> > > So, (with the faulty names) this patch results in some better generated
> > > code? You might want to show that in the patch message, to motivate
> > > this change a bit :-)
> >
> > The patch allows you to create additional conversions should the backend
> want to.
> > However it does not change the codegen of the backend.
> >
> > The patch simply allows a target to implement multiple conversions from the
> same
> > Source type. So you can now implement QI -> SI and QI -> HI and the
> vectorizer
> > will choose one based on costing.
>
> In what way? This is just confusing me more.
>
Before the change, the optab widen_ssum only took one mode, and so the
implementer
was required to pick which conversion they supported. So if you had V18QI as
your input,
but can do widening sum into both V8HI and V4SI you had to pick one.
With this change, you can now convey the exact from -> to summation your target
can do,
so if you support both, you can now implement both
widen_ssumv4siv18qi3 and widen_ssumv8hiv18qi3, whereas before because it was
just
widen_ssumv18qi3 you had to pick.
This is why this patch is a simple mechanical updating of the pattern already
defined in the
target to *explicitly* add the wider mode to the name.
> > The change is mechanical, and done over all targets that have implemented
> > this optab to keep them working.
>
> "Optab". You keep using that word. What does it mean?
>
> We are talking about a named RTL pattern here, AFAICS.
>
Optabs are the mechanism of how named patterns are defined. There are many
kinds of
them and the different kinds determine how resolution of the named pattern are
performed.
See optabs.def and optabs.cc. Some optabs have special expansion rules, like
this one, which
expands through expand_widen_pattern_expr. That's why your previous
" and one with two SImode and onbe DImode ops" is invalid. It's against the
semantics of widening
summation operations.
> I'll look at the patch you sent the URL to now.
Ok, thanks, and here's an updated changelog:
This patch is a mechanical rewrite of the widen_[us]sum optabs from a direct to
a conversion optab. The result of which requires the output mode to be added to
the existing patterns.
No change in functionality is expected.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
PR middle-end/122069
* config/rs6000/altivec.md (widen_usum<mode>3): Rename ...
(widen_usumv4si<mode>3): ... to this.
(widen_ssumv16qi3): Rename ...
(widen_ssumv4siv16qi3): ... to this.
(widen_ssumv8hi3): Rename ...
(widen_ssumv4siv8hi3): ... to this.
-- inline copy of patch --
diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
index
7edc288a65658c608a361caaa29a9f3fc2907b79..fa3368079ada661f3f159e27a3cf793698df1760
100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -3772,7 +3772,7 @@ (define_expand "sdot_prodv4siv8hi"
DONE;
})
-(define_expand "widen_usum<mode>3"
+(define_expand "widen_usumv4si<mode>3"
[(set (match_operand:V4SI 0 "register_operand" "=v")
(plus:V4SI (match_operand:V4SI 2 "register_operand" "v")
(unspec:V4SI [(match_operand:VIshort 1 "register_operand"
"v")]
@@ -3786,7 +3786,7 @@ (define_expand "widen_usum<mode>3"
DONE;
})
-(define_expand "widen_ssumv16qi3"
+(define_expand "widen_ssumv4siv16qi3"
[(set (match_operand:V4SI 0 "register_operand" "=v")
(plus:V4SI (match_operand:V4SI 2 "register_operand" "v")
(unspec:V4SI [(match_operand:V16QI 1 "register_operand"
"v")]
@@ -3800,7 +3800,7 @@ (define_expand "widen_ssumv16qi3"
DONE;
})
-(define_expand "widen_ssumv8hi3"
+(define_expand "widen_ssumv4siv8hi3"
[(set (match_operand:V4SI 0 "register_operand" "=v")
(plus:V4SI (match_operand:V4SI 2 "register_operand" "v")
(unspec:V4SI [(match_operand:V8HI 1 "register_operand" "v")]
rb19874.patch
Description: rb19874.patch
