Hi Bernd,
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".
> @@ -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.
Here, that outer block (around all of seven lines of code) is there just
to make a new scope, so that we *can* have a new local variable. It is
local. There is no "shadowing". It has the same name as some other
variables, but so what?
> - int i = XVECLEN (i3pat, 0) - 1;
> + int j = XVECLEN (i3pat, 0) - 1;
Why? No. No no no.
> @@ -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.
> @@ -2714,7 +2712,6 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn
> int maxreg;
> rtx_insn *temp_insn;
> rtx temp_expr;
> - struct insn_link *link;
Please don't mix moving scope (like this) with deletions (like the
previous), and all the renamings, and some re-indenting.
There is no description for this patch at all either, so it takes a long
time to read it.
> - HOST_WIDE_INT pos = INTVAL (XEXP (SET_DEST (x), 2));
> - unsigned HOST_WIDE_INT len = INTVAL (XEXP (SET_DEST (x), 1));
> + HOST_WIDE_INT pos1 = INTVAL (XEXP (SET_DEST (x), 2));
> + unsigned HOST_WIDE_INT len1 = INTVAL (XEXP (SET_DEST (x), 1));
No, and no. No "1" please. If the shadowing is an actual problem,
*improve* the code (like factor it a bit perhaps, much in combine is way
too big functions), instead of making it much more terrible (if I see a
"len1" I go look for a "len" or "len0" or "len2", and such are not to be
found here; the var should just be called "len").
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.
Segher
