fixed_scalar_and_varying_struct_p and varies_p

[Richard Sandiford]

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?

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: cc1-changes.diff
Type: text/x-diff
Size: 6898 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20120102/34a87c74/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-fs-and-vs-p.diff
Type: text/x-diff
Size: 18945 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20120102/34a87c74/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-scalar-in-struct-p.diff
Type: text/x-diff
Size: 16876 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20120102/34a87c74/attachment-0002.bin>