Bug 54154 - cprop_hardreg generates redundant instructions
Summary: cprop_hardreg generates redundant instructions
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.6.3
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-01 14:33 UTC by Paulo J. Matos
Modified: 2012-08-02 10:05 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Before cprop_hardreg (2.21 KB, application/octet-stream)
2012-08-01 14:33 UTC, Paulo J. Matos
Details
After cprop_hardreg (2.08 KB, application/octet-stream)
2012-08-01 14:34 UTC, Paulo J. Matos
Details
Add debug info when redundant insn is going to be generated (279 bytes, patch)
2012-08-01 14:37 UTC, Paulo J. Matos
Details | Diff
regcprop patch to remove redundant moves (842 bytes, patch)
2012-08-02 09:58 UTC, Paulo J. Matos
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo J. Matos 2012-08-01 14:33:43 UTC
Created attachment 27922 [details]
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.
Comment 1 Paulo J. Matos 2012-08-01 14:34:48 UTC
Created attachment 27923 [details]
After cprop_hardreg
Comment 2 Paulo J. Matos 2012-08-01 14:37:53 UTC
Created attachment 27924 [details]
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
Comment 3 Paulo J. Matos 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;

....
Comment 4 Paulo J. Matos 2012-08-01 15:01:29 UTC
Due to my last comment I marked this as a request for enhancement.
Comment 5 Paulo J. Matos 2012-08-02 09:34:03 UTC
I have now a patch for this which I will submit shortly.
Comment 6 Paulo J. Matos 2012-08-02 09:58:55 UTC
Created attachment 27926 [details]
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.
Comment 7 Paulo J. Matos 2012-08-02 10:05:40 UTC
(In reply to comment #6)
> With send it to gcc-patches.

's/With/Will/'