Re: [PATCH 4/8] rs6000: Consolidate target built-ins code

[Segher Boessenkool <segher@kernel.crashing.org>]

On Mon, Jan 31, 2022 at 04:01:58PM -0600, Bill Schmidt wrote:
> On 1/31/22 3:32 PM, Segher Boessenkool wrote:
> > On Fri, Jan 28, 2022 at 11:50:22AM -0600, Bill Schmidt wrote:
> >> Overloading support remains in rs6000-c.cc.
> > So, what is needed to move that as well?  Is moving that in the plan?
> 
> No, as explained above, that code needs to stay in the "special" file
> that the build machinery understands.  It looks very difficult to
> tease that apart, so I've given up on that.  Sorry!

Heh.  And you don't see that as a challenge?  Oh well, I could try :-)

> >> +/* Support targetm.vectorize.builtin_mask_for_load.  */
> >> +tree altivec_builtin_mask_for_load;
> > "Support"?  What does that mean?  Please describe what this tree is.
> 
> That comment is just moved.

Yes, but that isn't clear in your patch (series).

> This entire patch is just moving code.

Not according to the changelog!

It is hard to find the five, six, ten places with modification in an
1800-line patch (or what was it).

> I already separated that out
> from everything else.  The only exception is the one I called out at
> the beginning, that removing the builtin_mode_to_type[] array should
> have been done previously, and it's convenient to just delete it as
> part of this patch.
> 
> So I can change these other things that you mention, but that isn't
> really the point of this patch, so...  I guess please advise. :-)

Please make it less work for me to review your patches than it took you
to write them!  This often means you have to spend a bit more time and
thought on it.  As a side effect that makes better patches, you have
something to win here as well (apart from your patches being reviewed
easier :-) )


Segher