Patchwork Emit more REG_EQUIV notes for function args (PR42235)

login
register
mail settings
Submitter Bernd Schmidt
Date July 14, 2010, 11:14 a.m.
Message ID <4C3D9C06.60901@codesourcery.com>
Download mbox | patch
Permalink /patch/58865/
State New
Headers show

Comments

Bernd Schmidt - July 14, 2010, 11:14 a.m.
When moving arguments into pseudos, we are being very careful not to
emit any instructions that could possibly clobber other argument
registers.  Currently, we generate unnecessarily complicated sequences
of code for simple zero or sign extensions.

With this patch, we check can_extend_p and the necessary predicates to
see if an extend insn is available for the conversion we have to do.  If
so, we emit the insn directly, and create a REG_EQUIV note of the form
(sign_extend (mem)).  Reload can't really do anything with these yet,
but there's an optimization in the register allocator to move argument
loads directly before their use if there's only one use.  On Thumb-2
this happens already, we seem to generate better code than before.
Unfortunately Thumb-1, which the PR is about, has some other problems
which I'll try to fix later.

Bootstrapped and regression tested on i686-linux.  Also regression
tested on arm-linux, with my three usual sets of options.  Ok?


Bernd
PR target/42235
	* function.c (assign_parm_setup_reg): When an optab for extending
	exists, emit the insn directly and create a REG_EQUIV note.
Jeff Law - July 14, 2010, 6:54 p.m.
On 07/14/10 05:14, Bernd Schmidt wrote:
> When moving arguments into pseudos, we are being very careful not to
> emit any instructions that could possibly clobber other argument
> registers.  Currently, we generate unnecessarily complicated sequences
> of code for simple zero or sign extensions.
>
> With this patch, we check can_extend_p and the necessary predicates to
> see if an extend insn is available for the conversion we have to do.
Right, but what guarantee do we have that the conversion insn doesn't 
clobber a function argument register?   ISTM that to be safe you 
actually have to scan the insns created by gen_extend_insn to ensure 
they don't clobber something important.

I'm not an expert on what ports do these days, but I did work on a port 
(mn10200) where conversion "insns" where implemented as special function 
calls under the hood.  I don't recall if we allowed those special 
function calls to have visible side effects, but if they did, they'd 
show up as clobbers/uses attached to the normal conversion insn.    Of 
course the mn102 is dead, but I think it's method for implementing 
conversions was valid and if another port were to do something similar 
it would likely not interact well with your change.


>   If
> so, we emit the insn directly, and create a REG_EQUIV note of the form
> (sign_extend (mem)).
Creating more REG_EQUIVs seems like a good idea to me.



>    Reload can't really do anything with these yet,
> but there's an optimization in the register allocator to move argument
> loads directly before their use if there's only one use.  On Thumb-2
> this happens already, we seem to generate better code than before.
> Unfortunately Thumb-1, which the PR is about, has some other problems
> which I'll try to fix later.
>    
In theory, given the REG_EQUIV note we ought to get an entry in 
reg_equiv_mem, which the code I'm working on knows it can use instead of 
shoving the pseudo into a stack slot.  Effectively, my code will 
rematerialize the argument from the equivalent memory location 
regardless of the number of uses.


Jeff
Bernd Schmidt - July 14, 2010, 9:30 p.m.
On 07/14/2010 08:54 PM, Jeff Law wrote:
> On 07/14/10 05:14, Bernd Schmidt wrote:
>> When moving arguments into pseudos, we are being very careful not to
>> emit any instructions that could possibly clobber other argument
>> registers.  Currently, we generate unnecessarily complicated sequences
>> of code for simple zero or sign extensions.
>>
>> With this patch, we check can_extend_p and the necessary predicates to
>> see if an extend insn is available for the conversion we have to do.
> Right, but what guarantee do we have that the conversion insn doesn't
> clobber a function argument register?   ISTM that to be safe you
> actually have to scan the insns created by gen_extend_insn to ensure
> they don't clobber something important.
> 
> I'm not an expert on what ports do these days, but I did work on a port
> (mn10200) where conversion "insns" where implemented as special function
> calls under the hood.  I don't recall if we allowed those special
> function calls to have visible side effects, but if they did, they'd
> show up as clobbers/uses attached to the normal conversion insn.    Of
> course the mn102 is dead, but I think it's method for implementing
> conversions was valid and if another port were to do something similar
> it would likely not interact well with your change.

Hmm, ok.  That's awful, but I kind of expected someone would say that.
Did this really happen for integer zero/sign extend, or only for
floating point stuff?

If necessary I can try to test for a single insn with single_set and
push it to the sequence otherwise.


Bernd
Bernd Schmidt - July 14, 2010, 9:47 p.m.
On 07/14/2010 08:54 PM, Jeff Law wrote:
> In theory, given the REG_EQUIV note we ought to get an entry in
> reg_equiv_mem, which the code I'm working on knows it can use instead of
> shoving the pseudo into a stack slot.  Effectively, my code will
> rematerialize the argument from the equivalent memory location
> regardless of the number of uses.

Reload can do that already, I think, but it's not prepared to handle a
(zero_extend (mem)) inside a REG_EQUIV note.  That should be reasonably
trivial to add if the reg is never set other than in the initializing insn.


Bernd
Jeff Law - July 14, 2010, 10:21 p.m.
On 07/14/10 15:47, Bernd Schmidt wrote:
> On 07/14/2010 08:54 PM, Jeff Law wrote:
>    
>> In theory, given the REG_EQUIV note we ought to get an entry in
>> reg_equiv_mem, which the code I'm working on knows it can use instead of
>> shoving the pseudo into a stack slot.  Effectively, my code will
>> rematerialize the argument from the equivalent memory location
>> regardless of the number of uses.
>>      
> Reload can do that already, I think, but it's not prepared to handle a
> (zero_extend (mem)) inside a REG_EQUIV note.  That should be reasonably
> trivial to add if the reg is never set other than in the initializing insn.
>    
reload does, but it's code that can be problematical.  For example, 
replacement of an unallocated pseudo with its equivalent can trigger 
secondary reloads, spills, etc.

My code creates a new pseudo/allocno for the various ranges where the 
unallocated pseudo is live then asks IRA to color those split-range 
pseudos/allocnos.  What we end up presenting to reload is cleaner; 
though often they result in the same final code.  Handling this case 
falls out nicely from the infrastructure for splitting ranges of 
unallocated pseudos without equivalent forms (it's probably a couple 
dozen lines of new code).

jeff
Jeff Law - July 14, 2010, 10:28 p.m.
On 07/14/10 15:30, Bernd Schmidt wrote:
> On 07/14/2010 08:54 PM, Jeff Law wrote:
>    
>> On 07/14/10 05:14, Bernd Schmidt wrote:
>>      
>>> When moving arguments into pseudos, we are being very careful not to
>>> emit any instructions that could possibly clobber other argument
>>> registers.  Currently, we generate unnecessarily complicated sequences
>>> of code for simple zero or sign extensions.
>>>
>>> With this patch, we check can_extend_p and the necessary predicates to
>>> see if an extend insn is available for the conversion we have to do.
>>>        
>> Right, but what guarantee do we have that the conversion insn doesn't
>> clobber a function argument register?   ISTM that to be safe you
>> actually have to scan the insns created by gen_extend_insn to ensure
>> they don't clobber something important.
>>
>> I'm not an expert on what ports do these days, but I did work on a port
>> (mn10200) where conversion "insns" where implemented as special function
>> calls under the hood.  I don't recall if we allowed those special
>> function calls to have visible side effects, but if they did, they'd
>> show up as clobbers/uses attached to the normal conversion insn.    Of
>> course the mn102 is dead, but I think it's method for implementing
>> conversions was valid and if another port were to do something similar
>> it would likely not interact well with your change.
>>      
> Hmm, ok.  That's awful, but I kind of expected someone would say that.
> Did this really happen for integer zero/sign extend, or only for
> floating point stuff?
>    
It was all the PSImode crap.

> If necessary I can try to test for a single insn with single_set and
> push it to the sequence otherwise.
>    
For the mn103, the conversions were single insns...

Ultimately,  I think you have to peek at the insn(s) and see what 
registers they set/clobber.

jeff

Patch

Index: function.c
===================================================================
--- function.c	(revision 162146)
+++ function.c	(working copy)
@@ -2861,10 +2861,12 @@  static void
 assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm,
 		       struct assign_parm_data_one *data)
 {
-  rtx parmreg;
+  rtx parmreg, validated_mem;
+  rtx equiv_stack_parm;
   enum machine_mode promoted_nominal_mode;
   int unsignedp = TYPE_UNSIGNED (TREE_TYPE (parm));
   bool did_conversion = false;
+  bool need_conversion, moved;
 
   /* Store the parm in a pseudoregister during the function, but we may
      need to do it in a wider mode.  Using 2 here makes the result
@@ -2891,10 +2893,46 @@  assign_parm_setup_reg (struct assign_par
 
   assign_parm_remove_parallels (data);
 
+  equiv_stack_parm = data->stack_parm;
+  validated_mem = validize_mem (data->entry_parm);
+
+  need_conversion = (data->nominal_mode != data->passed_mode
+		     || promoted_nominal_mode != data->promoted_mode);
+  moved = false;
+
   /* Copy the value into the register, thus bridging between
      assign_parm_find_data_types and expand_expr_real_1.  */
-  if (data->nominal_mode != data->passed_mode
-      || promoted_nominal_mode != data->promoted_mode)
+  if (need_conversion)
+    {
+      enum insn_code icode;
+      rtx op0, op1;
+
+      icode = can_extend_p (promoted_nominal_mode, data->passed_mode,
+			    unsignedp);
+
+      op0 = parmreg;
+      op1 = validated_mem;
+      if (icode != CODE_FOR_nothing
+	  && insn_data[icode].operand[0].predicate (op0, promoted_nominal_mode)
+	  && insn_data[icode].operand[1].predicate (op1, data->passed_mode))
+	{
+	  enum rtx_code code = unsignedp ? ZERO_EXTEND : SIGN_EXTEND;
+	  rtx insn;
+
+	  insn = gen_extend_insn (op0, op1, promoted_nominal_mode,
+				  data->passed_mode, unsignedp);
+	  emit_insn (insn);
+
+	  equiv_stack_parm = gen_rtx_fmt_e (code, GET_MODE (parmreg),
+					    equiv_stack_parm);
+	  moved = true;
+	}
+    }
+
+  if (moved)
+    /* Nothing to do.  */
+    ;
+  else if (need_conversion)
     {
       int save_tree_used;
 
@@ -2919,7 +2957,7 @@  assign_parm_setup_reg (struct assign_par
 
       rtx tempreg = gen_reg_rtx (GET_MODE (data->entry_parm));
 
-      emit_move_insn (tempreg, validize_mem (data->entry_parm));
+      emit_move_insn (tempreg, validated_mem);
 
       push_to_sequence2 (all->first_conversion_insn, all->last_conversion_insn);
       tempreg = convert_to_mode (data->nominal_mode, tempreg, unsignedp);
@@ -2949,7 +2987,7 @@  assign_parm_setup_reg (struct assign_par
       did_conversion = true;
     }
   else
-    emit_move_insn (parmreg, validize_mem (data->entry_parm));
+    emit_move_insn (parmreg, validated_mem);
 
   /* If we were passed a pointer but the actual value can safely live
      in a register, put it in one.  */
@@ -3034,7 +3072,7 @@  assign_parm_setup_reg (struct assign_par
 	}
       else if ((set = single_set (linsn)) != 0
 	       && SET_DEST (set) == parmreg)
-	set_unique_reg_note (linsn, REG_EQUIV, data->stack_parm);
+	set_unique_reg_note (linsn, REG_EQUIV, equiv_stack_parm);
     }
 
   /* For pointer data type, suggest pointer register.  */