[Bug middle-end/54154] New: cprop_hardreg generates redundant instructions

[Bug middle-end/54154] New: cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

             Bug #: 54154
           Summary: cprop_hardreg generates redundant instructions
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: Paulo.Matos@csr.com


Created attachment 27922
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27922
Before cprop_hardreg

Hello,

I have touched this subject before:
* http://gcc.gnu.org/ml/gcc/2010-08/msg00347.html
* http://gcc.gnu.org/ml/gcc/2011-03/msg00214.html

and it looks like a long standing issue so I am wary of reporting a bug but I
think I did find the culprit code. So, unless you consider this a feature
somehow I reckon it's a bug.

cprop_hardreg grabs an insn chain and through forward propagation of registers
ends up generating redundant passes where the src and dest are the same (same
regnumber, same mode).

Consider the program (obtained using creduce):
int a;
int fn1 ();
void fn2 ();
int fn3 ();
int
fn4 ()
{
    if (fn1 ())
        return 0;
    a = fn3 ();
    if (a != (1 << (32 - (9 + 9))) - 1)
        fn2 (0, a);
    return (1 << (32 - (9 + 9))) - 1;
}

Now, after compiling for my backend with -Os I get before cprop_hardreg (after
ce3) [real logs attached]:

#8     reg AL <- call fn1
#50/51 if_then_else AL != 0
         goto label 34

#12    AL <- call fn3
#13    AH <- AL
#14    mem sym a <- AH
#48/49 if_then_else AH == 16383
         goto label 38

#17    AL <- 0
#19    call fn2
#4     AL <- 16383
#43    jump label 20

label 34:
#3     AL <- 0
#45    jump label 20

label 38:
#5     AL <- AH

label 20:
       return

the number after '#' is the insn number.

cprop_hardreg decided to replace AH with AL so the top of cprop_hardreg shows:
rescanning insn with uid = 14.
deleting insn with uid = 14.
insn 14: replaced reg 0 with 1
insn 48: replaced reg 0 with 1
rescanning insn with uid = 48.
deleting insn with uid = 48.
rescanning insn with uid = 5.
deleting insn with uid = 5.
insn 5: replaced reg 0 with 1

reg 0 is AH and reg 1 is AL. 
When you replace in insn 5, AH by AL you get the insn AL <- AL and that's #5
after cprop_hardreg.

I find it strange that there's no code checking if set dest is equal to
replacement and if it is, simply remove insn.

I think this is a bug and should be fixed.

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #1 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-01 14:34:48 UTC ---
Created attachment 27923
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27923
After cprop_hardreg

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #2 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-01 14:37:53 UTC ---
Created attachment 27924
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27924
Add debug info when redundant insn is going to be generated

Looking at the gcc log header after running cprop_hardreg shows:

rescanning insn with uid = 14.
deleting insn with uid = 14.
insn 14: replaced reg 0 with 1
insn 48: replaced reg 0 with 1
rescanning insn with uid = 48.
deleting insn with uid = 48.
rescanning insn with uid = 5.
deleting insn with uid = 5.
oops, substitution makes dest same as src
insn 5: replaced reg 0 with 1

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

Paulo J. Matos <Paulo.Matos at csr dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

--- Comment #3 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-01 15:00:34 UTC ---
OK, so I just noted this is not really a bug but a missed optimization:
/* Assert that SRC has been copied to DEST.  Adjust the data structures to
reflect that SRC contains an older copy of the shared value.  */

static void
copy_value (rtx dest, rtx src, struct value_data *vd)
{
  unsigned int dr = REGNO (dest);
  unsigned int sr = REGNO (src);
  unsigned int dn, sn;
  unsigned int i;

  /* ??? At present, it's possible to see noop sets.  It'd be nice if this were
cleaned up beforehand...  */
  if (sr == dr)
    return;

....

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #4 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-01 15:01:29 UTC ---
Due to my last comment I marked this as a request for enhancement.

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #5 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-02 09:34:03 UTC ---
I have now a patch for this which I will submit shortly.

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #6 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-02 09:58:55 UTC ---
Created attachment 27926
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27926
regcprop patch to remove redundant moves

This patch seems to fix 54154.
I am not sure its generic enough to make it straight into GCC but it works with
our port and I ran all the tests I have access to without any regressions.

With send it to gcc-patches.

[Bug middle-end/54154] cprop_hardreg generates redundant instructions

[Paulo.Matos at csr dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54154

--- Comment #7 from Paulo J. Matos <Paulo.Matos at csr dot com> 2012-08-02 10:05:40 UTC ---
(In reply to comment #6)
> With send it to gcc-patches.

's/With/Will/'