Hi Segher,
On 10/5/19 10:08 AM, Segher Boessenkool wrote:
> 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".
>
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.
>
>> @@ -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.
> 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?
Declaring a name in an inner block that is already declared in an outer
block is called shadowing, declaring the same name twice in the same block
is a syntax error.
The disadvantage of shadowing, is when you copy code from one block to an
outer block and you use the same name, you want the same value or a syntax
error at least when that variable is not declared in the outer scope,
but that is not always the case, when the outer block has an identical
variable it has a totally different meaning. Also the code is harder
to follow (unless you know it already very good).
>
>> - 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.
So frankly
a
for (i= ;;)
{
int i;
do
while (i-- >= 0)
}
is quite confusing.
>
>> @@ -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.
Obviously I can rename this insn to (insn1 ?) if you want that.
Please say which name you want this insn to have.
>
>> @@ -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.
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?
>
>> - 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.
>
Yes, that is true, this cannot solve every problem.
Well if we do not agree that we want to have that warning
in general, then that would of course not make any sense.
Do we want to keep shadowing variables or do we want to get
rid of it?
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 think you can get an impression where the shadowing comes from,
by using a recent host compiler and build stage1,
with make CXXFLAGS="-Wshadow=local" (maybe you have to install
the rtl.h patch first, as these shadow everything else).
Bernd.
