Re: [PATCH] rs6000: Fix some issues related to Power10 fusion [PR104024]

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

On Wed, Dec 21, 2022 at 11:41:58AM +0800, Kewen.Lin wrote:
> on 2022/12/20 21:19, Segher Boessenkool wrote:
> > Sure, I understand that.  What I don't like is the generator program is
> > much too big and unstructured already, and this doesn't help at all; it
> > makes it quite a bit worse even.

> >> Good point, and I just noticed that we should check tune setting instead
> >> of TARGET_POWER10 here?  Something like:
> >>
> >> if (!(rs6000_isa_flags_explicit & OPTION_MASK_P10_FUSION))
> >>   {
> >>     if (processor_target_table[tune_index].processor == PROCESSOR_POWER10)
> >>       rs6000_isa_flags |= OPTION_MASK_P10_FUSION;
> >>     else
> >>       rs6000_isa_flags &= ~OPTION_MASK_P10_FUSION;
> >>   }
> > 
> > Yeah that looks better :-)
> 
> I'm going to test this and commit it first.  :)

Thanks!

> > Maybe you can restructure the Perl code a bit in a first patch, and then
> > add the insn condition?  If you're not comfortable with Perl, I'll deal
> > with it, just update the patch.
> 
> OK, I'll give it a try, TBH I just fixed the place for insn condition, didn't
> look into this script, with a quick look, I'm going to factor out the main
> body from the multiple level loop, do you have some suggestions on which other
> candidates to be restructured?

Anything that makes the code easier to understand, basically.

This stuff is by nature pretty hard to read, but making the code shorter
and/or less nested should make it easier to understand.  You will need
to have fewer local variables per function than there are total now,
that will help.

Btw, this script isn't so big at all, but the patches are hard to review
without converting this to a side-by-side comparison first.  There must
be some way to improve that, that is what I'm looking for :-)


Segher