Re: [PATCH] Even more revised store motion fix

[Daniel Berlin <dan at cgsoftware dot com>]

Graham Stott <grahams@redhat.com> writes:

> Dan,
> 
> I took a look at this patch after it was committed the x86 bootstrap
> failure was reported.
> 
> Here are a couple comments.
> 
>> --- 6289,6347 ----
>> 
>>   /* Store motion code.  */
>> 
>>   /* Used in computing the reverse edge graph bit vectors.  */
>>   static sbitmap * st_antloc;
>> 
>>   /* Global holding the number of store expressions we are dealing with.  */
>>   static int num_stores;
>> 
>> 
>> ! /* Mark which registers are used by the mem, in the sbitmap used. */
>> ! static int
>> ! mark_mem_regs (x, used)
>> !      rtx x;
>> !      sbitmap used;
> 
> mark_mem_regs needs a prototype.
> 
> 
>> *************** static void
>> *** 6922,6928 ****
>>   free_store_memory ()
>>   {
>>     free_ldst_mems ();
>> -
>>     if (ae_gen)
>>       sbitmap_vector_free (ae_gen);
>>     if (ae_kill)
>> --- 7068,7073 ----
>> *************** free_store_memory ()
>> *** 6935,6942 ****
>>       sbitmap_vector_free (pre_insert_map);
>>     if (pre_delete_map)
>>       sbitmap_vector_free (pre_delete_map);
>> -   if (reg_set_in_block)
>> -     sbitmap_vector_free (reg_set_in_block);
>> 
>>     ae_gen = ae_kill = transp = st_antloc = NULL;
>>     pre_insert_map = pre_delete_map = reg_set_in_block = NULL;
> Why does this still set reg_set_in_block to NULL its a memory leak!

No, reg_set_in_block was freed forever ago. Not that this should be there.

There are other small problems i noticed (in determining if store ops
were okay, we were ignoring loads in parallels).
However, none of these are the "real killer".
I fixed them all, still get the exact same failure.
In fact, it's worse than that.
In a fit of frustration, I rewrote the entire thing a few hours ago,
rather than rely on code that was 
semi-broken before to not be completely broken. It computes the
vectors using a completely different mechanism.
However, It must have been right to begin with (or all the compiler
algorithms i've seen lie), because I get the *exact* same failure.

Not that it's all *my* fault.
For starters, rtx_addr_varies_p lies.

rtx_addr_varies_p "Returns 1 if X refers to a memory location whose
address cannot be compared reliably with constant addresses".

I remember asking RTH if this means we'd have to check the registers
if the address doesn't vary, and he said no, we wouldn't.

If you try that, you get failures even earlier.

Same with rtx_unstable_p, which is supposed to say whether something
would be different at a different point in the program.

But ignoring all that, i'm starting to look at the actual store
insertion/deletion code. It's the one portion i haven't replaced.
Also, a possible red herring is that in all the cases i've seenwhere we are
generating wrong code,  the store insertions require critical edge splitting.

If I just make it delete redundant stores (IE what it does while
calculating the vectors), everything is fine.
We can bootstrap and make check on x86 okay.

So at least redundant store elimination works.



> 
> Graham

-- 
"I used to own an ant farm but had to give it up.  I couldn't
find tractors small enough to fit it.
"-Steven Wright