Re: [PATCH 6/6] rs6000: Clean up the various rlwinm patterns

["Maciej W. Rozycki" <macro at linux-mips dot org>]

On Sun, 10 May 2015, Segher Boessenkool wrote:

> >  This clearly renames rather than removing the `rlwinm' pattern, please 
> > correctly reflect that in ChangeLog.  Some other, unnamed patterns are 
> > given names rather than deleted as well, just as you've noted at the top.  
> > And none of the other changes are mentioned in your ChangeLog entry.
> 
> The changelog says it deletes the patterns with the old names.  Which it
> does.  And it adds the ones with the new names.  Which it does.  No one
> said changelogs are useful ;-)

 No one?  Well, I'm saying it now, then. :)

> >  Would you be able to split this change up further by any chance?  
> 
> I could, but it would be a lot of extra work.  First, I haven't done those
> steps in order (they aren't "steps" at all): I take one pattern, and fix
> it up, then the next, etc.

 Yeah, I know the pain, it's usually how it happens.  You start cleaning 
up things and then you find yourself having done a number conceptually 
independent changes that overlap one another in the source modified.  
I've been through it many times.

 The thing is this extra work of untangling is worth doing as it'll help 
the maintainer and other members of the community, including those 
investigating change history months if not years from now for one reason 
or another, understand what the consequences of conceptually individual 
changes are.

 Specifically which changes are obviously harmless (e.g. formatting 
changes or the addition of debug `*' pattern names) and which are 
potential trouble makers (e.g. the reordering of operands or folding 
`define_insn' and `define_split' into `define_insn_and_split'), that e.g. 
need to be taken into account when porting changes that may rely on them.  
When all the bits are lumped together, it's very difficult to decipher 
them and easy to miss something.

 So I've done such patch splitting and rearrangement many times, using 
various techniques.  For example it's often faster and easier if you 
hand-edit the patch being split into two rather than trying to reproduce 
the intermediate stage in the source being patched.  Other people may have 
other hints and tricks.

 Of course you'll want to keep the original final version of the file 
changed around and then you can compare it to the result of applying a 
patch series, to make sure both results are identical.

> But much worse: if you do tiny pattern changes like you suggest, I can
> guarantee you the patches _will_ mis-apply.  All of the time :-(

 Well, if you get the line numbers recorded in patches right, which is how 
`diff' generates them, then it won't happen.  And as a committer you can 
verify you applied your own patches correctly, by taking a diff against 
your original patched version before pushing the changes into the repo. 
Especially when other upstream source changes have since caused line 
numbers to shift.

> > change addresses a single issue only.  That would avoid problems with 
> > ChangeLog and make the review easier.
> 
> This isn't the first patch like this, I've cleaned up most other PowerPC
> integer patterns already.  It's enough work already.  Often the changes
> cannot be separated, and even if they can you then need them in the fixed
> order you made for them, etc.

 Of course changes can be separated (if they cannot, then they weren't 
really conceptually separate in the first place), and you do need to get 
the order right.  E.g. patches that are potentially of interest to older 
release branches need to go first, followed by ones that are meant to stay 
on trunk only.

 I'll leave it up to David to decide anyway as he's the port maintainer, 
and I'm only a member of the community who happened to poke at this port 
once or twice in the past.

  Maciej