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

[Segher Boessenkool <segher at kernel dot crashing dot org>]

On Sun, May 10, 2015 at 07:02:33PM +0100, Maciej W. Rozycki wrote:
> > -(define_insn "rlwinm"
> > +(define_insn "*ashlsi3_imm_mask"
> >    [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
> >  	(and:SI (ashift:SI (match_operand:SI 1 "gpc_reg_operand" "r")
> >  			   (match_operand:SI 2 "const_int_operand" "i"))
> 
>  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 ;-)

>  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.

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 :-(

[ Sometimes two patterns even have the same name, and sometimes even 100%
identical contents.  Exciting! ]

> Perhaps into the very steps you listed at the top so that each individual 
> 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.

For this small patch I could probably do a better changelog though.


Segher