Ping.
> -----Original Message----- > From: Tamar Christina > Sent: 13 October 2025 08:49 > To: Segher Boessenkool <[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] > > > -----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..fa3368079ada661f3f159e > 27a3cf793698df1760 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")]
