[patch]: COMMITTED Enhance rtl-dse to remove unaligned read after writes.

Richard Sandiford rsandifo@nildram.co.uk
Mon Sep 17 22:24:00 GMT 2007


BTW, I applied the patch after Paolo's approval, but before I got
this message.

Kenneth Zadeck <zadeck@naturalbridge.com> writes:
> Richard Sandiford wrote:
>> We should also skip access_sizes that are smaller than the store mode
>> if truncating the store mode to the access size needs real insns.
>> The patch below does this with:
>>
>>       /* Try a wider mode if truncating the store mode to ACCESS_SIZE
>> 	 bytes requires a real instruction.  */
>>       if (access_size < GET_MODE_SIZE (store_mode)
>> 	  && !TRULY_NOOP_TRUNCATION (access_size * BITS_PER_UNIT,
>> 				     GET_MODE_BITSIZE (store_mode)))
>> 	continue;
>>   
> Given that store_mode is loop invariant, i would have written this
> code differently.  I would have added code before the loop like
>
> if (!TRULY_NOOP_TRUNCATION (access_size * BITS_PER_UNIT,
>
> 				     GET_MODE_BITSIZE (store_mode)))
> 	while (access_size < GET_MODE_SIZE store_mode)
> 	  access_size *= 2;

There's no requirement AFAIK that !TRULY_NOOP_TRUNCATION will remain
true for all access_sizes < GET_MODE_SIZE (store_mode).  E.g. a 64-bit
target is in theory allowed to require truncations for SI->HI without
requiring them for DI->SI.  If you really want to avoid the continue,
we could just have:

      while (access_size < GET_MODE_SIZE (store_mode)
             && !TRULY_NOOP_TRUNCATION (access_size * BITS_PER_UNIT,
                                        GET_MODE_BITSIZE (store_mode)))
        access_size *= 2;

inside the loop.

>>   - Fixing that showed up a df-related problem.  If a call to
>>     expand_binop generates more than one instruction, expand_binop
>>     helpfully adds a REG_EQUAL note to the final instruction.
>>     set_unique_reg_note then passes this insn to df_notes_rescan,
>>     and we'll end up scanning the instruction even if we decide to
>>     discard it.  This leads to an ICE.
>>
>>     I believe the right fix here is to set DF_NO_INSN_RESCAN
>>     while expanding the sequence.  If we decide to keep the
>>     instructions, we'll still pass them df_insn_rescan from
>>     emit_insn_before.  (This is of course how the scan is queued
>>     for insns without a REG_EQUAL note.)
>>
>>     The infrastructure would probably be more robust if scans were
>>     only automatically scheduled for the topmost sequence.  However,
>>   
> This may be hard because there is no 100% reliable way to tell if an
> insn is IN the function or whether you just created it and are playing
> air guitar with it.  In df_insn_rescan, we check that the insn has a
> basic block to before we queue the insn to be rescanned.
>
> I think that the proper fix is to copy that check for the bb into
> df_notes_rescan.  Since the insns have not been inserted yet, they do
> not have a bb.

OK, I've bootstrapped & regression-tested the patch below on
x86_64-linux-gnu.  OK to install?

Richard


gcc/
	* df-scan.c (df_notes_rescan): Do nothing if the instruction does
	not yet have a basic block.
	* dse.c (find_shift_sequence): Don't set DF_NO_INSN_RESCAN.

Index: gcc/df-scan.c
===================================================================
--- gcc/df-scan.c	2007-09-17 22:58:31.000000000 +0100
+++ gcc/df-scan.c	2007-09-17 22:59:02.000000000 +0100
@@ -2004,6 +2004,10 @@ df_notes_rescan (rtx insn)
   if (df->changeable_flags & DF_NO_INSN_RESCAN)
     return;
 
+  /* Do nothing if the insn hasn't been emitted yet.  */
+  if (!BLOCK_FOR_INSN (insn))
+    return;
+
   df_grow_bb_info (df_scan);
   df_grow_reg_info ();
 
Index: gcc/dse.c
===================================================================
--- gcc/dse.c	2007-09-17 22:58:37.000000000 +0100
+++ gcc/dse.c	2007-09-17 22:59:02.000000000 +0100
@@ -1429,10 +1429,8 @@ find_shift_sequence (rtx read_reg,
       /* In theory we could also check for an ashr.  Ian Taylor knows
 	 of one dsp where the cost of these two was not the same.  But
 	 this really is a rare case anyway.  */
-      df_set_flags (DF_NO_INSN_RESCAN);
       target = expand_binop (new_mode, lshr_optab, new_reg,
 			     GEN_INT (shift), new_reg, 1, OPTAB_DIRECT);
-      df_clear_flags (DF_NO_INSN_RESCAN);
 
       shift_seq = get_insns ();
       end_sequence ();



More information about the Gcc-patches mailing list