diff mbox

RFA: Merge definitions of get_some_local_dynamic_name

Message ID 87egukhwmg.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Oct. 7, 2014, 12:50 p.m. UTC 
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>> Hi Richard,
>>> Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>>>> Hi Richard,
>>>>>> It seems the new get_some_local_dynamic_name implementation in
>>>>>> function.c lost the non-NULL check the sparc.c version had.  I'm
>>>>>> currently testing the following patch:
>>>>>
>>>>> Could you do a:
>>>>>
>>>>>   call debug_rtx (...)
>>>>>
>>>>> on the insn that contains a null pointer?  Normally insn patterns
>>>>> shouldn't contain nulls, so I was wondering whether this was some
>>>>> SPARC-specific construct.
>>>>
>>>> proved a bit difficult to do: at the default -O2, insn was optimized
>>>> away, at -g3 -O0, I only got
>>>>
>>>> can't compute CFA for this frame
>>>>
>>>> with gdb 7.8 even after recompiling all of the gcc dir with -g3 -O0.
>>>>
>>>> Here's what I find after inserting the call in the source:
>>>>
>>>> (insn 30 6 28 (sequence [
>>>>             (call_insn:TI 8 6 7 (parallel [
>>>>                         (set (reg:SI 8 %o0)
>>>>                             (call (mem:SI (unspec:SI [
>>>>                                             (symbol_ref:SI ("__tls_get_addr"))
>>>>                                         ] UNSPEC_TLSLDM) [0  S4 A32])
>>>>                                 (const_int 1 [0x1])))
>>>>                         (clobber (reg:SI 15 %o7))
>>>>                     ]) /vol/gcc/src/hg/trunk/local/libgo/runtime/proc.c:936 390 {tldm_call32}
>>>>                  (expr_list:REG_EH_REGION (const_int -2147483648 [0xffffffff80000000])
>>>>                     (nil))
>>>>                 (expr_list (use (reg:SI 8 %o0))
>>>>                     (nil)))
>>>>             (insn 7 8 28 (set (reg:SI 8 %o0)
>>>>                     (plus:SI (reg:SI 23 %l7)
>>>>                         (unspec:SI [
>>>>                                 (reg:SI 8 %o0 [112])
>>>>                             ] UNSPEC_TLSLDM))) 388 {tldm_add32}
>>>>                  (nil))
>>>>         ]) /vol/gcc/src/hg/trunk/local/libgo/runtime/proc.c:936 -1
>>>>      (nil))
>>>
>>> Bah, a sequence.  Hadn't thought of that.
>>>
>>> IMO it's a bug for a walk on a PATTERN to pull in non-PATTERN parts
>>> of an insn.  We should really be looking at the patterns of the two
>>> subinsns instead and ignore the other stuff.  Let me have a think
>>> about it.
>>
>> did you come to a conclusion here?
>
> Sorry, forgot to come back to this.  I have a patch that iterates over
> PATTERNs of a SEQUENCE if the SEQUENCE (rather than its containing insn)
> is the topmost iterated rtx.  So if PATTERN (insn) is a SEQUENCE:
>
>    FOR_EACH_SUBRTX (...., insn, x)
>      ...
>
> will iterate over the insns in the SEQUENCE (including pattern, notes,
> jump label, etc.), whereas:
>
>    FOR_EACH_SUBRTX (...., PATTERN (insn), x)
>      ...
>
> would only iterate over the patterns of the insns in the SEQUENCE.

Does this work for you?  I tested it on x86_64-linux-gnu but obviously
that's not particularly useful for SEQUENCEs.

Thanks,
Richard

gcc/
	* rtlanal.c (generic_subrtx_iterator <T>::add_subrtxes_to_queue):
	Add the parts of an insn in reverse order, with the pattern at
	the top of the queue.  Detect when we're iterating over a SEQUENCE
	pattern and in that case just consider patterns of subinstructions.

Comments

Rainer Orth Oct. 8, 2014, 7:50 a.m. UTC | #1 
Hi Richard,

> Does this work for you?  I tested it on x86_64-linux-gnu but obviously
> that's not particularly useful for SEQUENCEs.

the patch is fine, as tested on both sparc-sun-solaris2.11 and (for good
measure) i386-pc-solaris2.11.

Thanks.
        Rainer
diff mbox

Patch

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	2014-09-25 16:40:44.944406590 +0100
+++ gcc/rtlanal.c	2014-10-07 13:13:57.698132753 +0100
@@ -128,29 +128,58 @@  generic_subrtx_iterator <T>::add_subrtxe
 						    value_type *base,
 						    size_t end, rtx_type x)
 {
-  const char *format = GET_RTX_FORMAT (GET_CODE (x));
+  enum rtx_code code = GET_CODE (x);
+  const char *format = GET_RTX_FORMAT (code);
   size_t orig_end = end;
-  for (int i = 0; format[i]; ++i)
-    if (format[i] == 'e')
-      {
-	value_type subx = T::get_value (x->u.fld[i].rt_rtx);
-	if (__builtin_expect (end < LOCAL_ELEMS, true))
-	  base[end++] = subx;
-	else
-	  base = add_single_to_queue (array, base, end++, subx);
-      }
-    else if (format[i] == 'E')
-      {
-	int length = GET_NUM_ELEM (x->u.fld[i].rt_rtvec);
-	rtx *vec = x->u.fld[i].rt_rtvec->elem;
-	if (__builtin_expect (end + length <= LOCAL_ELEMS, true))
-	  for (int j = 0; j < length; j++)
-	    base[end++] = T::get_value (vec[j]);
-	else
-	  for (int j = 0; j < length; j++)
-	    base = add_single_to_queue (array, base, end++,
-					T::get_value (vec[j]));
-      }
+  if (__builtin_expect (INSN_P (x), false))
+    {
+      /* Put the pattern at the top of the queue, since that's what
+	 we're likely to want most.  It also allows for the SEQUENCE
+	 code below.  */
+      for (int i = GET_RTX_LENGTH (GET_CODE (x)) - 1; i >= 0; --i)
+	if (format[i] == 'e')
+	  {
+	    value_type subx = T::get_value (x->u.fld[i].rt_rtx);
+	    if (__builtin_expect (end < LOCAL_ELEMS, true))
+	      base[end++] = subx;
+	    else
+	      base = add_single_to_queue (array, base, end++, subx);
+	  }
+    }
+  else
+    for (int i = 0; format[i]; ++i)
+      if (format[i] == 'e')
+	{
+	  value_type subx = T::get_value (x->u.fld[i].rt_rtx);
+	  if (__builtin_expect (end < LOCAL_ELEMS, true))
+	    base[end++] = subx;
+	  else
+	    base = add_single_to_queue (array, base, end++, subx);
+	}
+      else if (format[i] == 'E')
+	{
+	  unsigned int length = GET_NUM_ELEM (x->u.fld[i].rt_rtvec);
+	  rtx *vec = x->u.fld[i].rt_rtvec->elem;
+	  if (__builtin_expect (end + length <= LOCAL_ELEMS, true))
+	    for (unsigned int j = 0; j < length; j++)
+	      base[end++] = T::get_value (vec[j]);
+	  else
+	    for (unsigned int j = 0; j < length; j++)
+	      base = add_single_to_queue (array, base, end++,
+					  T::get_value (vec[j]));
+	  if (code == SEQUENCE && end == length)
+	    /* If the subrtxes of the sequence fill the entire array then
+	       we know that no other parts of a containing insn are queued.
+	       The caller is therefore iterating over the sequence as a
+	       PATTERN (...), so we also want the patterns of the
+	       subinstructions.  */
+	    for (unsigned int j = 0; j < length; j++)
+	      {
+		typename T::rtx_type x = T::get_rtx (base[j]);
+		if (INSN_P (x))
+		  base[j] = T::get_value (PATTERN (x));
+	      }
+	}
   return end - orig_end;
 }