This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: fixed_scalar_and_varying_struct_p and varies_p


On Mon, Jan 2, 2012 at 3:42 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Thanks for both replies.
>
> Richard Guenther <richard.guenther@gmail.com> writes:
>> On Thu, Dec 29, 2011 at 8:48 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>>> fixed_scalar_and_varying_struct_p passes an _address_ rather than a MEM.
>>>> So in these cases fixed_scalar_and_varying_struct_p effectively becomes
>>>> a no-op on targets that don't allow MEMs in addresses and takes on
>>>> suspicious semantics for those that do. ?In the former case, every
>>>> address is treated as "unvarying" and f_s_a_v_s_p always returns null.
>>>> In the latter case, things like REG addresses are (wrongly) treated as
>>>> unvarying while a MEM address might correctly be treated as varying,
>>>> leading to false positives.
>>>>
>>>> It looks like this goes back to when fixed_scalar_and_varying_struct_p
>>>> was added in r24759 (1999).
>>>
>>> Does this mean that MEM_IN_STRUCT_P and MEM_SCALAR_P have also been
>>> effectively disabled since then?
>
> Some important callers (cse.c and sched-deps.c) do use the proper
> varies_p routine, so it probably isn't quite that extreme. ?But...
>
>>>> AIUI, the true_dependence varies_p parameter exists for the benefit
>>>> of CSE, so that it can use its local cse_rtx_varies_p function.
>>>> All other callers should be using rtx_varies_p instead. ?Question is,
>>>> should I make that change, or is it time to get rid of
>>>> fixed_scalar_and_varying_struct_p instead?
>>>
>>> I'd vote for the latter (and for eliminating MEM_IN_STRUCT_P and MEM_SCALAR_P
>>> in the process, if the answer to the above question is positive), there is no
>>> point in resurrecting this now IMO.
>>
>> I agree. ?The tree level routines should be able to figure most of, if
>> not all, cases
>> on their own via rtx_refs_may_alias_p (similar to the
>> nonoverlapping_component_refs
>> case which we could simply delete as well).
>
> ...that's 2 votes for and none so far against. :-)
>
> I compiled the cc1 .ii files on x86_64-linux-gnu with and without
> fixed_scalar_and_varying_struct_p. ?There were 19 changes in total,
> all of them cases where sched2 was able to reorder two memory accesses
> because of f_s_a_v_s_p. ?I've attached the diff below.
>
> A good example is:
>
> ?if (bit_obstack)
> ? ?{
> ? ? ?element = bit_obstack->elements;
>
> ? ? ?if (element)
> ? ? ? ?/* Use up the inner list first before looking at the next
> ? ? ? ? ? element of the outer list. ?*/
> ? ? ? ?if (element->next)
> ? ? ? ? ?{
> ? ? ? ? ? ?bit_obstack->elements = element->next;
> ? ? ? ? ? ?bit_obstack->elements->prev = element->prev;
> ? ? ? ? ?}
> ? ? ? ?else
> ? ? ? ? ?/* ?Inner list was just a singleton. ?*/
> ? ? ? ? ?bit_obstack->elements = element->prev;
> ? ? ?else
> ? ? ? ?element = XOBNEW (&bit_obstack->obstack, bitmap_element);
> ? ?}
> ?else
> ? ?{
> ? ? ?element = bitmap_ggc_free;
> ? ? ?if (element)
> ? ? ? ?/* Use up the inner list first before looking at the next
> ? ? ? ? ? element of the outer list. ?*/
> ? ? ? ?if (element->next)
> ? ? ? ? ?{
> ? ? ? ? ? ?bitmap_ggc_free = element->next;
> ? ? ? ? ? ?bitmap_ggc_free->prev = element->prev;
> ? ? ? ? ?}
> ? ? ? ?else
> ? ? ? ? ?/* ?Inner list was just a singleton. ?*/
> ? ? ? ? ?bitmap_ggc_free = element->prev;
> ? ? ?else
> ? ? ? ?element = ggc_alloc_bitmap_element_def ();
> ? ?}
>
> from bitmap.c, specifically:
>
> ? ? ? ? ? ?bitmap_ggc_free = element->next;
> ? ? ? ? ? ?bitmap_ggc_free->prev = element->prev;
>
> Without f_s_a_v_s_p, sched2 couldn't tell that element->prev didn't
> alias bitmap_ggc_free. ?And this in turn is because cfgcleanup
> considered trying to merge this block with:
>
> ? ? ? ? ? ?bit_obstack->elements = element->next;
> ? ? ? ? ? ?bit_obstack->elements->prev = element->prev;
>
> It called merge_memattrs for each pair of instructions that it was
> thinking of merging, and because the element->next and element->prev
> MEMs were based on different SSA names, we lost the MEM_EXPR completely.
>
> As it happens, we decided not to merge the blocks after all.
> So an obvious first observation is that query functions like
> flow_find_cross_jump and flow_find_head_matching_sequence shouldn't
> change the rtl. ?We should only do that once we've decided which
> instructions we're actually going to merge.
>
> Of course, that's not a trivial change. ?It's easy to make
> try_head_merge_bb call merge_memattrs during merging, but less
> easy for try_crossjump_to_edge and cond_exec_process_if_block.
> (Note that the latter, like try_head_merge_bb, can end up merging
> fewer instructions than flow_find_* saw).
>
> But does the choice of SSA name actually count for anything this
> late on? ?Should we consider MEM_EXPRs node_X->prev and node_Y->prev
> to be "similar enough", if node_X and node_Y have equal types?

It's a conservative way of merging alias info.  A more precise variant
would union the points-to information of both bases (and stick it to
a new SSA name), conveniently pt_solution_ior_into exists.
And no, we can't just pick either SSA name.

Back in time we decided it wasn't that important to try hard to
preserve alias info for this case...

I'd say open a missed optimization bug with the testcase and go ahead
with both patches.  Let's see if Eric has some comments first though.

Thanks,
Richard.

> I've attached a patch to remove fixed_scalar_and_varying_struct_p
> just in case it's OK. ?Tested on mips64-linux-gnu.
>
> Also, as Eric says, this is really the only middle-end use of
> MEM_SCALAR and MEM_IN_STRUCT_P. ?It looks like the only other
> use is in config/m32c/m32c.c:m32c_immd_dbl_mov. ?TBH, I don't
> really understand what that function is trying to test, so I can't
> tell whether it should be using MEM_EXPR instead.
>
> I've attached a patch to remove MEM_IN_STRUCT_P and MEM_SCALAR too,
> although I don't think the m32c.c change is acceptable. ?Tested again
> on mips64-linux-gnu.
>
> Richard
>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]