Re: RFC/RFA: Fix bug with REE optimization corrupting extended registers

[Jeff Law <law at redhat dot com>]

On 11/18/2015 07:15 AM, Nick Clifton wrote:
> Hi Guys,
>
>    I recently discovered a bug in the current Redundant Extension
>    Elimination optimization.  If the candidate extension instruction
>    increases the number of hard registers used, the pass does not check
>    to see if these extra registers are live between the definition and
>    the extension.
>
>    For example:
>
>      (insn  44 (set (reg:QI r11) (mem:QI (reg:HI r20)))
>      (insn  45 (set (reg:QI r10) (mem:QI (reg:HI r18)))
>      [...]
>      (insn  71 (set (reg:HI r14) (zero_extend:HI (reg:QI r11)))
>      [...]
>      (insn  88 (set (reg:HI r10) (zero_extend:HI (reg:QI r10)))
>
>    (This is on the RL78 target where HImode values occupy two hard
>    registers and QImode values only one.  The bug however is generic, not
>    RL78 specific).
>
>    The REE pass transforms this into:
>
>      (insn  44 (set (reg:QI r11) (mem:QI (reg:HI r20)))
>      (insn  45 (set (reg:HI r10) (zero_extend:HI (mem:QI (reg:HI r18))))
>      [...]
>      (insn  71 (set (reg:HI r14) (zero_extend:HI (reg:QI r11)))
>      [...]
>      (insn  88 deleted)
>
>    Note how the new set at insn 45 clobbers the value loaded by insn 44
>    into r11.
>
>    The patch below is my attempt to fix this.  It is not correct
>    however.  I could not work out how to determine if a given hard
>    register is live at any point between two insns.  So instead I used
>    the liveness information associated with the basic block containing
>    the definition.  If one of the extended registers is live or becomes
>    live in this block, and this block is not the same block as the one
>    containing the extension insn, then I stop the optimization.  This
>    works for the RL78 for all of the cases that I could find.
>
>    So ... comments please ?
>
>    Will gcc ever generate a single basic block that contains both the
>    definition and the extension instructions, but also where the extra
>    hard registers are used for another purpose as well ?
>
>    Tested with no regressions (or fixes) on an x86-pc-linux-gnu target.
>    Also tested with no regression and 7 fixes on an rl78-elf target.
>
> Cheers
>    Nick
>
> gcc/ChangeLog
> 2015-11-18  Nick Clifton  <nickc@redhat.com>
>
> 	* ree.c (regs_live_between): New function.
>          (add_removable_extension): If the extension uses more hard
>          registers than the definition then check that the extra hard
>          registers are not live between the definition and the
>          extension.
>
> @@ -1080,6 +1123,30 @@
>   	      }
>   	  }
>
> +      /* Fourth, if the extended version occupies more registers than
> +	 the original version then make sure that these extra registers
> +	 are not live between the definition and the extension.  */
> +      if (HARD_REGNO_NREGS (REGNO (dest), mode)
> +	  > HARD_REGNO_NREGS (REGNO (reg), GET_MODE (reg)))
So this seems like a reasonable place to catch this.  I certainly prefer 
to avoid work later by filtering earlier like you've done.

The problem is with your implementation of regs_live_between.  You're 
assuming we've got straight-line code between the original set and the 
extension.  That's not guaranteed.  Essentially we've got use-def chains 
via the DF framework.  So, in theory, they can be in different blocks 
and arbitrary flow control between them.  In fact, the extension could 
be before the original set in the insn chain (though obviously not for 
execution order).

I think it's reasonable to just punt when we see that the source/dest 
register of the extension are the same and the number of hard regs is 
different.  That's what I'm testing now.

jeff