Hi Bernd,
On Sat, Oct 05, 2019 at 09:12:19AM +0000, Bernd Edlinger wrote:
> On 10/5/19 10:08 AM, Segher Boessenkool wrote:
> > I'll just review the combine part.
> >
> > On Sat, Oct 05, 2019 at 06:36:34AM +0000, Bernd Edlinger wrote:
> >> --- gcc/combine.c (revision 276598)
> >> +++ gcc/combine.c (working copy)
> >> @@ -1219,7 +1219,7 @@ combine_instructions (rtx_insn *f, unsigned int nr
> >> FOR_BB_INSNS (this_basic_block, insn)
> >> if (INSN_P (insn) && BLOCK_FOR_INSN (insn))
> >> {
> >> - rtx links;
> >> + rtx link;
> >
> > Where/what does this conflict with? Well, nothing; I meant "shadow", it
> > shadows some other name. This whole (nested) loop should just be factored
> > out.
> >
> > It's not a good name either way: it is *not* what we call "links" in
> > combine, it is a pointer to a register note instead. So name it
> > something like "note"?
> >
> > Renaming code without considering its semantics is called "obfuscation".
>
> All I can consider is if the variable called links is of the same type,
> and if it is live or dead at this point.
> I have no idea how a good name should be, unfortunately.
> I'd be happy with using note, obviously.
You can change *this* code to non-sensical or at least worse names, or you
can fix the actual problems (and the latter cannot be done mechanically).
The former is arguably wrong, and because of that, certainly not an
obvious fix. That is true about everything here, not just combine.
> >> @@ -1542,10 +1542,10 @@ retry:
> >> reg_stat.release ();
> >>
> >> {
> >> - struct undo *undo, *next;
> >> - for (undo = undobuf.frees; undo; undo = next)
> >> + struct undo *undo, *next1;
> >> + for (undo = undobuf.frees; undo; undo = next1)
> >
> > No. I object.
> >
> > If there is anything called "next" in a bigger scope as well, and for some
> > reason we feel that this is bad, then *that* one should change, and change
> > to a name that is meaningful in all of its large scope.
>
> I would take your suggestion, if any, but I would be happy
> if you want to fix that, it will obviously way better when
> you do it, and we are of course not in a hurry.
You need to factor this code (not "refactor", that is something else),
or change the name in the *outer* scope. Or, you can say this isn't a
big problem at all, also a reasonable stance if you ask me, so no "fix"
is needed.
> The disadvantage of shadowing, is when you copy code from one block to an
> outer block and you use the same name,
Stop right there. Your problem is your code has too big blocks, so *that*
is what needs fixing there.
> >> - int i = XVECLEN (i3pat, 0) - 1;
> >> + int j = XVECLEN (i3pat, 0) - 1;
> >
> > Why? No. No no no.
>
> this is in this loop:
>
> for (i = 0; i < XVECLEN (PATTERN (insn), 0); i++)
> {
>
> so using i in the inner block is not okay, because a human
> reader may thing that i of the outer loop is assigned a new
> value here.
Using "i" in the inner loop is just dandy. If it then conflicts with some
name in some larger scope, you need to fix *that*: change *that* name, or
don't have that scope accessible at all (i.e., factor your code).
> >> @@ -2546,8 +2546,6 @@ update_cfg_for_uncondjump (rtx_insn *insn)
> >> delete_insn (insn);
> >> if (EDGE_COUNT (bb->succs) == 1)
> >> {
> >> - rtx_insn *insn;
> >> -
> >> single_succ_edge (bb)->flags |= EDGE_FALLTHRU;
> >>
> >> /* Remove barriers from the footer if there are any. */
> >
> > This is a separate change. Although, if this would be an unused variable
> > someone would have noticed by now. So what is this about?
> >
> > Are you suggesting abusing a variable in a larger scope for some local
> > use? That doesn't fly. Rename the var in the larger scope, instead. To
> > a *useful* *descriptive* name.
>
> We use scratch values, like
> rtx_insn *insn;
> everywhere, so i did not see
> what is wrong with just using it.
The outer "insn" is a function argument here! Reusing that var for
something else is a real problem, much worse than having the same name
so someone might just *think* you do this.
> Obviously I can rename this insn to (insn1 ?) if you want that.
> Please say which name you want this insn to have.
I want it to be called "insn". "insn" is a good name for it.
> > There is no description for this patch at all either, so it takes a long
> > time to read it.
>
> It took me a while to write this up:
>
> * combine.c (combine_instructions, can_combine_p): Rename local vars.
> (update_cfg_for_uncondjump, try_combine): Remove shadowing local vars.
> (find_split_point): Rename local vars.
> (combine_simplify_rtx, make_compound_operation, if_then_else_cond,
> simplify_shift_const_1): Remove shadowing local vars.
> (simplify_comparison): Rename local var. Remove shadowing local var.
> (get_last_value_validate): Remove shadowing local vars.
> (move_deaths): Rename local vars.
>
> Not good enough?
That is a changelog entry. Changelogs say *what* changed, not *why*. To
review a patch, we need to know the *why*. Not just the problem you try
to fix, but also why you decided to do it this way (and not other ways),
etc. Changelogs are helpful when reviewing a patch, but not much for
understanding a change, not at all for figuring out the motivation behind
a change.
> > So. You say that shadowing is a problem. A problem for what? Most of
> > the time it is completely harmless. If you write in non-ancient style
> > (so write shortish functions and blocks, and declare every var at its
> > first use) you don't see actual shadowing problems much at all.
> >
> >
> > In most cases here the warning indicates that the code it too big (and
> > too old), it could use a bit of factoring and rewriting.
> >
> > But all this patch does is shut up the warnings, without fixing the causes.
> > This is not an improvement. Hiding problems is bad.
>
> Yes, that is true, this cannot solve every problem.
I am saying this patch causes bigger problems than it solves.
> Well if we do not agree that we want to have that warning
> in general, then that would of course not make any sense.
The warning is fine, but should not be enabled by default (could be in -W
perhaps?), and yes I question the value for the GCC code itself as well.
> Do we want to keep shadowing variables or do we want to get
> rid of it?
Like I said, this warning often points to code that is badly structured.
Spaghetti. This makes it a useful warning, esp. when modifying such code.
But renaming the stuff in the inner scope makes it worse code, and just
hides the deeper problems.
> In general the functions here are way too large, and we should
> just re-factor the functions to be shorter.
>
> Would you like to take a look at how to fix this file, I could
> definitely need some help?
I am maintainer of combine, I know all about its many problems (it has much
deeper problems than this unfortunately). Thanks for your help though, this
is much appreciated, but I do think your current patch is not a step in the
right direction.
Segher
