Re: PATCH commited to dataflow branch.

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Ulrich,

I am replying to both of your emails at one time. 

Thanks for looking at this so carefully.
Ulrich Weigand wrote:
> Kenny Zadeck wrote:
>
>   
>> Part of this code contains fixes that we made to reload to make pre and
>> post modify work properly.  I was wondering if you would check to see if
>> we did this in an approved manner.  Congrats on becoming a reload
>> maintainer!!!
>>     
>
> Thanks ;-)
>
>   
>> The issue is that if you have (premodify r1 (add r1 + 6)) and reload
>> wants to substitute in
>> r2 + 10 for r1.  This substitution must not be allowed because the
>> result (premodify r2 + 10 (add r2 + 16)) means that every time the load
>> or store that the premodify is attached to is accessed, the index
>> variable will be moved by an improper amount.
>>
>> I do not believe that what we have done in this patch is the best way to
>> go because it requires support in the legitimate_address implementation
>> in the port to stop something that has nothing to do with the port.  
>> However, David Edelsohn and myself are not reload professionals and this
>> was the only thing we could make work.  
>>     
>
> I'll have to have a closer look how auto-inc is supposed to be
> handled in reload, I'm not all that familiar with that part yet.
> However, I do have the distinct impression that register elimination
> inside auto-inc constructs is just fundamentally broken.  While
> your patch may happen to fix some instances, I don't quite see how
> it is a complete solution.
>
> Now, the reason why this didn't show up previously is clearly that
> the original auto-inc construction pass in flow.c would specifically
> avoid to ever create an auto-inc of any (even potentially) eliminable
> register:
>
> static void
> attempt_auto_inc (struct propagate_block_info *pbi, rtx inc, rtx insn,
>                   rtx mem, rtx incr, rtx incr_reg)
> {
> [snip]
>   if (dead_or_set_p (incr, incr_reg)
>       /* Mustn't autoinc an eliminable register.  */
>       && (regno >= FIRST_PSEUDO_REGISTER
>           || ! TEST_HARD_REG_BIT (elim_reg_set, regno)))
>     {
>       /* This is the simple case.  Try to make the auto-inc.  If
>          we can't, we are done.  Otherwise, we will do any
>          needed updates below.  */
>       if (! validate_change (insn, &XEXP (mem, 0), inc, 0))
>         return;
>     }
>
>
>   
This is what the comments says but the effect of this code is not really
what it seems.
There are several places in flow that can generate auto-inc-dec and this
code just stops it in one place.   Note that there is no test in the
else part of this block nor is there any code to do this in
try_pre_increment (the other code path that generates pre and post
increment insn.) I spent a lot of time scratching my head about this
code when I wrote my new version because it seems so ineffective.  I
concluded that it must just be rot but I will admit that the plumbing in
reload does not seem to work there is a flaw in my logic somewhere (or
else latent bugs).

> Note the /* Mustn't autoinc an eliminable register.  */ comment.
>
> The elim_reg_set is constructed thus (in life_analysis):
>
>   /* Record which registers will be eliminated.  We use this in
>      mark_used_regs.  */
>
>   CLEAR_HARD_REG_SET (elim_reg_set);
>
> #ifdef ELIMINABLE_REGS
>   for (i = 0; i < (int) ARRAY_SIZE (eliminables); i++)
>     SET_HARD_REG_BIT (elim_reg_set, eliminables[i].from);
> #else
>   SET_HARD_REG_BIT (elim_reg_set, FRAME_POINTER_REGNUM);
> #endif
>
> so it simply holds any register appearing as "from" of any
> elimination rule.
>
>
> It looks like this test was introduced by Richard Henderson
> on 2000-04-14 (revision 33150) as a fix to bugs related to
> handling of eliminable registers inside auto-inc in reload.
>
> Now, while we might certainly think on how to teach reload
> to handle such cases in the future, the conservative fix
> for your problem would appear to be to add an equivalent
> check to your new auto-inc pass.
>
>
> Bye,
> Ulrich
>
>   
Ulrich Weigand wrote:
> David Edelsohn wrote:
>
>   
>> 	If elimination is applied to both operands and an invalid address
>> is created, legitimate_address should kick it out later.  It always is the
>> responsibility of the port to specify what addressing forms are
>> legitimate.  The current behavior of reload allows other invalid addresses
>> not involving PRE_MODIFY to be formed at that same point and
>> legitimate_address discovers the problem later, so this is not doing
>> anything new.  The port is the only place in the compiler with knowledge
>> to determine if an address like this is invalid.  Also, elimination could
>> create a valid address.
>>     
>
> Unfortunately, things are a bit more complicated in the interaction 
> between eliminate_regs and find_reloads.  In particular, it is not
> the case that legitimate_address is called at any point to query
> whether or not the result of register elimination is valid.
>
> The process fundamentally goes like this:
>
> - at the beginning of reload, all addresses in the insn stream
>   satisfy legitimate_address in the non-strict sense.
>
> - eliminate_regs makes some changes to addresses, without
>   verifying whether the results are valid
>
>   
In the patch that we sent, we added code to eliminate_regs_1 for the pre
and post modify cases. 

What if we replaced that code with code that checked to see if there was
a non zero offset
on the elimination?  If the offset was non zero would return x (which
appears to mean do not do the elimination), otherwise we would return
the elimination of the operands as we do now in the patch.

This would have the effect that index register would not be eliminated
but there seem to be many other cases in this function that also return x.
> - find_reloads_address is called on any address, checks if the
>   address satisfies legitimate_address in the strict sense
>
> - if not, it pushes a bunch of reloads that will have the effect
>   of making the address strictly legitimate  (*)
>
> - at the end of reload, all addresses in the insn stream satisfy
>   legitimate_address in the strict sense (this is never explicitly
>   verified, however!)
>
>
> Note the step (*) is not allowed to fail, it *must* be able to
> make the address strictly legitimate.  The current implementation
> of find_reloads_address is able to do that, because there is a
> very limited set of ways just how an address may fail to be
> strictly legitimate, and the function contains code to deal with
> all of these cases.  However, that is only true when making
> certain assumptions on what exactly eliminate_regs is allowed
> to do to an address!  If register elimination generates addresses
> that are invalid in a way find_reloads_address does not expect,
> it will silently generate invalid code -- this has nothing to
> do with whether or not the target legitimate_address check
> will reject such addresses.
>
>
> Bye,
> Ulrich
>
>   

Kenny