[Bug middle-end/42691] New: Wrong REG_EQUAL note is added to SUBREG node

[Bug middle-end/42691] New: Wrong REG_EQUAL note is added to SUBREG node

[jingyu at google dot com]

A wrong REG_EQUAL is attached to SUBREG during forward propagation.

Before fwprop1 
(insn 56 55 57 3 double.cpp:12 (set (reg/v:DI 180 [ _D_inf ])
        (const_int 0 [0x0])) 164 {*thumb1_movdi_insn} (nil))
(insn 60 59 61 3 double.cpp:12 (set (reg:SI 189)
        (const_int 2146435072 [0x7ff00000])) 168 {*thumb1_movsi_insn} (nil))
(insn 62 61 63 3 double.cpp:12 (set (subreg:SI (reg/v:DI 180 [ _D_inf ]) 4)
        (reg:SI 189)) 168 {*thumb1_movsi_insn} (nil))

After fwprop1, a note "REG_EQUAL (const_int 2146435072 [0x7ff00000]" is
attached to insn 62.

(insn 56 54 60 3 double.cpp:12 (set (reg/v:DI 180 [ _D_inf ])
        (const_int 0 [0x0])) 164 {*thumb1_movdi_insn} (nil))
(insn 60 56 62 3 double.cpp:12 (set (reg:SI 189)
        (const_int 2146435072 [0x7ff00000])) 168 {*thumb1_movsi_insn} (nil))

(insn 62 60 63 3 double.cpp:12 (set (subreg:SI (reg/v:DI 180 [ _D_inf ]) 4)
        (reg:SI 189)) 168 {*thumb1_movsi_insn} (expr_list:REG_EQUAL (const_int
2
146435072 [0x7ff00000])
        (nil)))

Then at combine pass, when the three insns are combined into one insn, the
operand of the set note becomes [0x7ff0000000000000], but the REG_EQUAL note is
still [0x7ff00000].

(insn 62 155 63 3 double.cpp:12 (set (reg/v:DI 180 [ _D_inf ])
        (const_int 9218868437227405312 [0x7ff0000000000000])) 164
{*thumb1_movdi_insn} (expr_list:REG_EQUAL (const_int 2146435072 [0x7ff00000])
        (nil)))

If the wrong REG_EQUAL is not used in later optimization, then nothing wrong is
observed. However, we find a case that the bug is triggered at ira pass.

What happens is at ira pass, in reload() (reload1.c), gcc tries to find "SET
REG(regno) = const N". When gcc finds one, it records constant equivalent in
reg_equiv_constant so that the use of the register will be substituted by
find_reloads. gcc puts "reg_equiv_constant[regno]=x". However, notice that, "x"
here is the value from the REG_EQUAL note, not the SET operand. In our case,
the value in reg 180 is 0x7ff0000000000000, but gcc says
reg_equiv_constant[180]=0x7ff00000. Later, when gcc tries to simplify the
SUBREG for insn 65, it picks the wrong value in reg_equiv_const[180] and passes
the wrong value to insn 65.

(insn 65 64 66 3 double.cpp:13 (set (reg:DF 2 r2)
        (subreg:DF (reg/v:DI 180 [ _D_inf ]) 0)) 183 {*thumb_movdf_insn} (nil))

becomes

(insn 65 64 66 3 double.cpp:13 (set (reg:DF 2 r2)
        (const_double:DF 1.06047983010398252661938461763305382790433594771e-314 
[0x0.ffep-1043])) 183 {*thumb_movdf_insn} (nil))

The const_double in insn 65 is wrong.


test case double.cpp:

union _D_rep {
  unsigned short rep[4];
  double val;
};

int add(double* key, double* table)
{
  unsigned i = 0;
  double* deletedEntry = 0;
  while (1) {
    double* entry = table + i;
    _D_rep _D_inf = {{ 0, 0, 0, 0x7ff0 }};
    if (*entry == _D_inf.val)
      break;

    if (*entry == *key)
      return 0;

    _D_rep _D_inf2 = {{ 0, 0, 0, 0x7ff0 }};
    if (_D_inf2.val)
      deletedEntry = entry;

    i++;
  }
  if (deletedEntry)
    return 1;
  return 0;
}

The command is:
arm-eabi-g++ -Os -mthumb double.cpp -c -o double.o


-- 
           Summary: Wrong REG_EQUAL note is added to SUBREG node
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jingyu at google dot com
 GCC build triplet: X86_64-linux-gnu
  GCC host triplet: X86_64-linux-gnu
GCC target triplet: arm-unknown-eabi


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

[Bug middle-end/42691] Wrong REG_EQUAL note is added to SUBREG node

[jingyu at google dot com]

------- Comment #1 from jingyu at google dot com  2010-01-11 19:59 -------
I forgot to paste the wrong assembly code.

arm-eabi-g++ -Os -mthumb double.cpp --save-temps -c -o double.o

The assembly for the following part
    _D_rep _D_inf = {{ 0, 0, 0, 0x7ff0 }};
    if (*entry == _D_inf.val)
      break;
is
        ldr     r3, .L10+4  ;;  _D_inf higher part
        ldr     r2, .L10  ;; _D_inf lower part
        mov     r0, r5
        mov     r1, r6
        bl      __aeabi_dcmpeq

.L10:
        .word   2146435072  ;; 0x7ff00000
        .word   0

The value loaded for _D_inf is wrong.
It is a little-endian target.


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[ebotcazou at gcc dot gnu dot org]

------- Comment #2 from ebotcazou at gcc dot gnu dot org  2010-01-11 22:00 -------
The REG_EQUAL note is correct after fwprop but not after combine, so the
problem lies in combine.  The note should have been removed by
adjust_for_new_dest since the destination of I3 has changed.


-- 

ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu dot
                   |                            |org
          Component|middle-end                  |rtl-optimization
            Summary|Wrong REG_EQUAL note is     |problematic REG_EQUAL note
                   |added to SUBREG node        |added to SUBREG


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]



------- Comment #3 from jingyu at google dot com  2010-01-12 02:45 -------
Subject: Re:  problematic REG_EQUAL note added to 
        SUBREG

On Mon, Jan 11, 2010 at 2:00 PM, ebotcazou at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #2 from ebotcazou at gcc dot gnu dot org  2010-01-11 22:00 -------
> The REG_EQUAL note is correct after fwprop but not after combine, so the
> problem lies in combine.  The note should have been removed by
> adjust_for_new_dest since the destination of I3 has changed.
>

Unfortunately, the note is not removed by adjust_for_new_dest.

I traced gcc and saw how it combined insn 156 and insn 62.

First, gcc works out a new pattern "newpat" which sets
0x7ff0000000000000 to reg 180 and jumps to "validate_replacement",
where it checks if the result of combination a valid instruction.

line3061:  /* Is the result of combination a valid instruction?  */
line3062:  insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);

In our case, insn_code_number is 164.

Then gcc comes to line3657. Since changed_i3_dest is 0, gcc does not
call adjust_for_new_dest at all.

line3657:
if (changed_i3_dest)
    {
      PATTERN (i3) = newpat;
      adjust_for_new_dest (i3);
    }

After this, there is no chance where the note of i3 can be removed.

I did not read the entire combine pass. So I am not sure whether it is
fine to just add something like this after line3062 where
recog_for_combine is called.

          if (insn_code_number >= 0)
            changed_i3_dest = 1;

Another option to fix this problem is not to attach REG_EQUAL note to
"set subreg reg" at all during forward propagation.
Index: fwprop.c
==============================
=====================================
--- fwprop.c    (revision 155510)
+++ fwprop.c    (working copy)
@@ -1297,7 +1297,7 @@ forward_propagate_and_simplify (df_ref u
       recognized.  Either we're already replacing in the note, or
       we'll separately try plugging the definition in the note and
       simplifying.  */
-      set_reg_equal = (note == NULL_RTX);
+      set_reg_equal = (note == NULL_RTX && REG_P (SET_DEST (use_set)));
   }

 if (GET_MODE (*loc) == VOIDmode)

I have tested this patch on arm-unknown-eabi target. There is no regression.

What do you think?

Thanks,
Jing

>
> --
>
> ebotcazou at gcc dot gnu dot org changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                 CC|                            |ebotcazou at gcc dot gnu dot
>                   |                            |org
>          Component|middle-end                  |rtl-optimization
>            Summary|Wrong REG_EQUAL note is     |problematic REG_EQUAL note
>                   |added to SUBREG node        |added to SUBREG
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
>


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[ebotcazou at gcc dot gnu dot org]

------- Comment #4 from ebotcazou at gcc dot gnu dot org  2010-01-12 08:57 -------
> Then gcc comes to line3657. Since changed_i3_dest is 0, gcc does not
> call adjust_for_new_dest at all.
> 
> line3657:
> if (changed_i3_dest)
>     {
>       PATTERN (i3) = newpat;
>       adjust_for_new_dest (i3);
>     }

We should try and set changed_i3_dest to 1 in this particular case as well.

Where does the dest get changed exactly?

> I did not read the entire combine pass. So I am not sure whether it is
> fine to just add something like this after line3062 where
> recog_for_combine is called.
> 
>           if (insn_code_number >= 0)
>             changed_i3_dest = 1;

Too big a hammer, the dest of I3 is generally not changed.

> Another option to fix this problem is not to attach REG_EQUAL note to
> "set subreg reg" at all during forward propagation.

Too big a hammer as well, the note is correct at this point.


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[ramana at gcc dot gnu dot org]

-- 

ramana at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu dot
                   |                            |org
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
      Known to fail|                            |4.4.2 4.5.0
           Priority|P3                          |P2
   Last reconfirmed|0000-00-00 00:00:00         |2010-01-12 09:33:45
               date|                            |


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]



------- Comment #5 from jingyu at google dot com  2010-01-12 22:21 -------
Subject: Re:  problematic REG_EQUAL note added to 
        SUBREG

On Tue, Jan 12, 2010 at 12:57 AM, ebotcazou at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #4 from ebotcazou at gcc dot gnu dot org  2010-01-12 08:57 -------
>> Then gcc comes to line3657. Since changed_i3_dest is 0, gcc does not
>> call adjust_for_new_dest at all.
>>
>> line3657:
>> if (changed_i3_dest)
>>     {
>>       PATTERN (i3) = newpat;
>>       adjust_for_new_dest (i3);
>>     }
>
> We should try and set changed_i3_dest to 1 in this particular case as well.
>
> Where does the dest get changed exactly?
>

In our example, GCC first sets the new pattern to "newpat". "newpat"
actually points to the pattern of i2.
At line3750, gcc records i3's note into "i3notes" and propagates the
note if shared.
Then at line3775, gcc assigns newpat to i3.
At line3775 (combine.c):
    INSN_CODE (i3) = insn_code_number;
    PATTERN (i3) = newpat;
At line3837,  REG_NOTES (i3) = 0;
At line3879, "i3notes" is distributed back to i3.


>> I did not read the entire combine pass. So I am not sure whether it is
>> fine to just add something like this after line3062 where
>> recog_for_combine is called.
>>
>>           if (insn_code_number >= 0)
>>             changed_i3_dest = 1;
>
> Too big a hammer, the dest of I3 is generally not changed.


>
>> Another option to fix this problem is not to attach REG_EQUAL note to
>> "set subreg reg" at all during forward propagation.
>
> Too big a hammer as well, the note is correct at this point.
>
>
> --
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
>


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

------- Comment #6 from jingyu at google dot com  2010-01-13 01:18 -------
Subject: Re:  problematic REG_EQUAL note added to 
        SUBREG

> We should try and set changed_i3_dest to 1 in this particular case as well.

Here is the patch which only changes changed_i3_dest for two
particular cases, where the dest of i3 really changed. The test case
attached to this bug is case2.

case 1:
   i1 = 0. i2=(parallel [(set p, ?)...]), i3=(set q, p)
    -> newpat=(parallel[(set q, ?),...])
    -> i3.pattern=newpat

case2:
   i1 = 0, i2=(set p, q), i3=(set subreg(s, reg(p), offset), a)
     ->newpat=(set p, <new value>)
     -> i3.pattern=newpat



Index: combine.c
===================================================================
--- combine.c   (revision 155801)
+++ combine.c   (working copy)
@@ -2531,6 +2531,10 @@ try_combine (rtx i3, rtx i2, rtx i1, int

              newpat = p2;
              i3_subst_into_i2 = 1;
+
+              /* If the result of the combination is recognized, the note of
i3
+               * needs to be removed. */
+              changed_i3_dest = 1;
              goto validate_replacement;
            }
     }
@@ -2667,6 +2671,10 @@ try_combine (rtx i3, rtx i2, rtx i1, int
                 immed_double_const (olo, ohi, GET_MODE (SET_DEST (temp))));

          newpat = PATTERN (i2);
+
+          /* If the result of combination is recognized, the note of
i3 needs to
+           * to be removed. */
+          changed_i3_dest = 1;
          goto validate_replacement;
        }
     }
@@ -3060,6 +3068,9 @@ try_combine (rtx i3, rtx i2, rtx i1, int
   /* Is the result of combination a valid instruction?  */
   insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);

+  if (insn_code_number < 0)
+      changed_i3_dest = 0;
+
   /* If the result isn't valid, see if it is a PARALLEL of two SETs where
      the second SET's destination is a register that is unused and isn't
      marked as an instruction that might trap in an EH region.  In that case,

This patch fixes the bug.
Do you think if this patch is favorable? If yes, I will do dejagnu
test and send it to gcc-patches for review.

Thanks,
Jing


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[ebotcazou at gcc dot gnu dot org]

------- Comment #7 from ebotcazou at gcc dot gnu dot org  2010-01-13 10:03 -------
> This patch fixes the bug.
> Do you think if this patch is favorable? If yes, I will do dejagnu
> test and send it to gcc-patches for review.

The patch is probably correct, but I'd ditch hunks #1 and #3.  In case1, the
code already sets a flag (i3_subst_into_i2) so we're better off not setting
another one, lest it badly interacts with the former.  Moreover, even if the
pattern is not valid, the dest of I3 has nevertheless changed.

So hunk #2 alone is sufficient, but the comment isn't quite up to the point. 
I'd copy and adapt for case2 the comment of case1 instead:

              /* Replace the dest in I2 with our dest and make the resulting
                 insn the new pattern for I3.  Then skip to where we
                 validate the pattern.  Everything was set up above.  */

as well as update the comment at the destination:

  /* We come here when we are replacing a destination in I2 with the
     destination of I3.  */
 validate_replacement:

which was written for case1 and obviously overlooks case2.

You reported the problem against 4.4.x as well and I think the patch would be
safe enough as to be put on the branch, but formally the bug needs to be a
regression from earlier compilers.  Do you know of such a compiler, e.g. 4.3.x,
which correctly compiles your testcase for the same target and with the same
options?


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]



------- Comment #8 from jingyu at google dot com  2010-01-13 20:03 -------
Yes. This is a regression from 4.2.1. I have not tried 4.3.x. But at least,
gcc-4.2.1 works correctly.

The following assembly is compiled out of gcc-4.2.1 with -mthumb -Os.

        ldr     r3, .L14+8
        ldr     r1, [r3]        <--- load 0x0
        ldr     r2, [r3, #4]   <--- load 0x07ff00000
        ...
        str     r1, [sp]
        str     r2, [sp, #4]
.L2:
        ldr     r4, [r6]
        ldr     r5, [r6, #4]
        ldr     r2, [sp]    <--- r2 = 0x0
        ldr     r3, [sp, #4]   <--- r3 = 0x07ff00000
        mov     r0, r4
        mov     r1, r5
        bl      __aeabi_dcmpeq
...

.L14:
        .word   0
        .word   0
        .word   _ZZ3addPdS_E3C.0
        .word   _ZZ3addPdS_E3C.1
...
_ZZ3addPdS_E3C.0:
        .short  0
        .short  0
        .short  0
        .short  32752


-- 

jingyu at google dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jingyu at google dot com
                   |dot org                     |
             Status|NEW                         |ASSIGNED
      Known to work|                            |4.2.1


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4809 bytes --]



------- Comment #9 from jingyu at google dot com  2010-01-13 22:56 -------
Subject: Re:  problematic REG_EQUAL note added to 
        SUBREG

On Wed, Jan 13, 2010 at 2:03 AM, ebotcazou at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #7 from ebotcazou at gcc dot gnu dot org  2010-01-13 10:03 -------
>> This patch fixes the bug.
>> Do you think if this patch is favorable? If yes, I will do dejagnu
>> test and send it to gcc-patches for review.
>
> The patch is probably correct, but I'd ditch hunks #1 and #3.  In case1, the
> code already sets a flag (i3_subst_into_i2) so we're better off not setting
> another one, lest it badly interacts with the former.  Moreover, even if the
> pattern is not valid, the dest of I3 has nevertheless changed.
>
I see. When i3_subst_into_i2, the i3notes are taken care of. I will
remove hunks #1.

> So hunk #2 alone is sufficient, but the comment isn't quite up to the point.
> I'd copy and adapt for case2 the comment of case1 instead:
>
>              /* Replace the dest in I2 with our dest and make the resulting
>                 insn the new pattern for I3.  Then skip to where we
>                 validate the pattern.  Everything was set up above.  */
>

Sounds reasonable. Done.

> as well as update the comment at the destination:
>
>  /* We come here when we are replacing a destination in I2 with the
>     destination of I3.  */
>  validate_replacement:
>
This comment is really out dated. I read most codes before this line.
There are many various cases coming to this line. It is pretty hard
for me to summarize all cases. Do you have any suggestion what can be
on the new comment?
Otherwise, I would simply remove this comment.

I does hunk #3, because I am not sure if all combination results from
case2 would pass "recog_for_combine". If any case fail, gcc may try
other combination patterns, where I am afraid changed_i3_dest=1 may
cause problem. I haven't read the code after validate_replacement
label thoroughly. To be conservative, I use hunk#3 to guarantee that
if the combination result from case2 is not recognized, nothing has
been changed. gcc is free to go ahead and try other combination
patterns. This is conservative. If anyone confirms that the cases that
I concern of will never happen, I am happy to remove hunk#3. Can you
confirm?

Index: combine.c
===================================================================
--- combine.c   (revision 155801)
+++ combine.c   (working copy)
@@ -2663,10 +2663,17 @@ try_combine (rtx i3, rtx i2, rtx i1, int
          i2dest = SET_DEST (temp);
          i2dest_killed = dead_or_set_p (i2, i2dest);

+         /* Replace the source in I2 with our dest and make the resulting
+            insn the new pattern for I3.  Then skip to where we
+            validate the pattern.  Everything was set up above.  */
          SUBST (SET_SRC (temp),
                 immed_double_const (olo, ohi, GET_MODE (SET_DEST (temp))));

          newpat = PATTERN (i2);
+
+          /* If the result of combination is recognized, the note of I3 needs
+           * to be removed. */
+          changed_i3_dest = 1;
          goto validate_replacement;
        }
     }
@@ -3038,8 +3045,6 @@ try_combine (rtx i3, rtx i2, rtx i1, int
        }
     }

-  /* We come here when we are replacing a destination in I2 with the
-     destination of I3.  */
  validate_replacement:

   /* Note which hard regs this insn has as inputs.  */
@@ -3060,6 +3065,11 @@ try_combine (rtx i3, rtx i2, rtx i1, int
   /* Is the result of combination a valid instruction?  */
   insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);

+  /* If the result of combination is not a valid instruction, reset
+   * changed_i3_dest, to be conservative.  */
+  if (insn_code_number < 0)
+      changed_i3_dest = 0;
+
   /* If the result isn't valid, see if it is a PARALLEL of two SETs where
      the second SET's destination is a register that is unused and isn't
      marked as an instruction that might trap in an EH region.  In that case,


I have done dejagnu tests. This patch does not introduce regression.

Thanks,
Jing

> which was written for case1 and obviously overlooks case2.
>
> You reported the problem against 4.4.x as well and I think the patch would be
> safe enough as to be put on the branch, but formally the bug needs to be a
> regression from earlier compilers.  Do you know of such a compiler, e.g. 4.3.x,
> which correctly compiles your testcase for the same target and with the same
> options?
>
>
> --
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
>


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[ebotcazou at gcc dot gnu dot org]

------- Comment #10 from ebotcazou at gcc dot gnu dot org  2010-01-13 23:27 -------
> Otherwise, I would simply remove this comment.

Fine with me, it's indeed a little confusing.

> I does hunk #3, because I am not sure if all combination results from
> case2 would pass "recog_for_combine". If any case fail, gcc may try
> other combination patterns, where I am afraid changed_i3_dest=1 may
> cause problem. I haven't read the code after validate_replacement
> label thoroughly. To be conservative, I use hunk#3 to guarantee that
> if the combination result from case2 is not recognized, nothing has
> been changed. gcc is free to go ahead and try other combination
> patterns. This is conservative. If anyone confirms that the cases that
> I concern of will never happen, I am happy to remove hunk#3. Can you
> confirm?

Yes, jumping to validate_replacement means that the other combinations are not
tried.  If the new pattern is not valid, the code will try to massage it and
not try other combinations.  And I think that, in practice, it won't actually
do anything since the various subsequent tricks are incompatible with case2.

> Index: combine.c
> ===================================================================
> --- combine.c   (revision 155801)
> +++ combine.c   (working copy)
> @@ -2663,10 +2663,17 @@ try_combine (rtx i3, rtx i2, rtx i1, int
>           i2dest = SET_DEST (temp);
>           i2dest_killed = dead_or_set_p (i2, i2dest);
> 
> +         /* Replace the source in I2 with our dest and make the resulting
> +            insn the new pattern for I3.  Then skip to where we
> +            validate the pattern.  Everything was set up above.  */

"Replace the source in I2 with the new constant and..."

>           SUBST (SET_SRC (temp),
>                  immed_double_const (olo, ohi, GET_MODE (SET_DEST (temp))));
>
>           newpat = PATTERN (i2);
> +
> +          /* If the result of combination is recognized, the note of I3 needs
> +           * to be removed. */
> +          changed_i3_dest = 1;

No need to invoke the consequences, just state the cause: "The dest of I3 has
been replaced with the dest of I2."


-- 


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

[Bug rtl-optimization/42691] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]



------- Comment #11 from jingyu at google dot com  2010-01-14 00:13 -------
Subject: Re:  problematic REG_EQUAL note added to 
        SUBREG

> Yes, jumping to validate_replacement means that the other combinations are not
> tried.  If the new pattern is not valid, the code will try to massage it and
> not try other combinations.  And I think that, in practice, it won't actually
> do anything since the various subsequent tricks are incompatible with case2.
>
Good to know.
Thanks very much for the explanation.
Hunk#3 is removed.

I will send the following patch to gcc-patches and CC you.

Index: combine.c
===================================================================
--- combine.c   (revision 155801)
+++ combine.c   (working copy)
@@ -2663,10 +2663,16 @@ try_combine (rtx i3, rtx i2, rtx i1, int
          i2dest = SET_DEST (temp);
          i2dest_killed = dead_or_set_p (i2, i2dest);

+         /* Replace the source in I2 with the new constant and make the
+            resulting insn the new pattern for I3.  Then skip to where we
+            validate the pattern.  Everything was set up above.  */
          SUBST (SET_SRC (temp),
                 immed_double_const (olo, ohi, GET_MODE (SET_DEST (temp))));

          newpat = PATTERN (i2);
+
+          /* The dest of I3 has been replaced with the dest of I2. */
+          changed_i3_dest = 1;
          goto validate_replacement;
        }
     }
@@ -3038,8 +3044,6 @@ try_combine (rtx i3, rtx i2, rtx i1, int
        }
     }

-  /* We come here when we are replacing a destination in I2 with the
-     destination of I3.  */
  validate_replacement:

   /* Note which hard regs this insn has as inputs.  */


Thanks,
Jing


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[rguenth at gcc dot gnu dot org]

-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.4.3


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at gcc dot gnu dot org]

------- Comment #12 from jingyu at gcc dot gnu dot org  2010-01-15 18:47 -------
Subject: Bug 42691

Author: jingyu
Date: Fri Jan 15 18:47:45 2010
New Revision: 155944

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155944
Log:
2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * combine.c (try_combine): Set changed_i3_dest to 1 when I2 and I3 set
        a pseudo to a constant and are merged, and adjust comments.

2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * gcc.c-torture/execute/pr42691.c: New.


Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr42691.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at gcc dot gnu dot org]

------- Comment #13 from jingyu at gcc dot gnu dot org  2010-01-15 19:09 -------
Subject: Bug 42691

Author: jingyu
Date: Fri Jan 15 19:08:53 2010
New Revision: 155945

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155945
Log:
Backport from mainline(4.5) to fix a regression bug.

2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * combine.c (try_combine): Set changed_i3_dest to 1 when I2 and I3 set
        a pseudo to a constant and are merged, and adjust comments.

2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * gcc.c-torture/execute/pr42691.c: New.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/gcc.c-torture/execute/pr42691.c
Modified:
    branches/gcc-4_4-branch/gcc/ChangeLog
    branches/gcc-4_4-branch/gcc/combine.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[sezeroz at gmail dot com]

------- Comment #14 from sezeroz at gmail dot com  2010-01-15 20:59 -------
For me, the testcase doesn't abort or exit successfully,
it just segfaults (i686, x86_64, gcc-4.3, 4.4).

$ gcc44 -g -O2 -Wall -W pr42691.c
$ gdb ./a.out
GNU gdb Fedora (6.8-24.fc9)
[...]
Program received signal SIGSEGV, Segmentation fault.
add (key=0xbf9eef38, table=0xbf9eef28) at pr42691.c:29
29        *deletedEntry = 0.0;


-- 

sezeroz at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sezeroz at gmail dot com


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[mikpe at it dot uu dot se]

------- Comment #15 from mikpe at it dot uu dot se  2010-01-15 21:11 -------
The test case added with this PR appears to be rather broken. On armv5tel with
gcc-4.3 or 4.4 (without the fix) -Os -mthumb I do get an abort(), which seems
deliberate. However, replacing -Os -mthumb with -O0 -g results in a segfault
because deletedEntry is NULL when "*deletedEntry = 0.0;" is executed. It also
segfaults in the same way on i686/x86_64/powerpc64 even with old non-broken
compiler versions.

Looking at the code I don't see how it could ever work, that is exit normally
with status 0.


-- 

mikpe at it dot uu dot se changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpe at it dot uu dot se


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]



------- Comment #16 from jingyu at google dot com  2010-01-15 21:14 -------
Subject: Re:  [4.4/4.5 regression] problematic 
        REG_EQUAL note added to SUBREG

Sorry. The following change would fix it on X86.

Index: testsuite/gcc.c-torture/execute/pr42691.c
===================================================================
--- testsuite/gcc.c-torture/execute/pr42691.c   (revision 155944)
+++ testsuite/gcc.c-torture/execute/pr42691.c   (working copy)
@@ -26,7 +26,8 @@

     i++;
   }
-  *deletedEntry = 0.0;
+  if (deletedEntry)
+    *deletedEntry = 0.0;
   return 0;
 }


On Fri, Jan 15, 2010 at 1:11 PM, mikpe at it dot uu dot se
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #15 from mikpe at it dot uu dot se  2010-01-15 21:11 -------
> The test case added with this PR appears to be rather broken. On armv5tel with
> gcc-4.3 or 4.4 (without the fix) -Os -mthumb I do get an abort(), which seems
> deliberate. However, replacing -Os -mthumb with -O0 -g results in a segfault
> because deletedEntry is NULL when "*deletedEntry = 0.0;" is executed. It also
> segfaults in the same way on i686/x86_64/powerpc64 even with old non-broken
> compiler versions.
>
> Looking at the code I don't see how it could ever work, that is exit normally
> with status 0.

The test case would fail on ARM -mthumb -Os due to the bug described
in the thread.

I will fix the problem immediately.

Jing

>
>
> --
>
> mikpe at it dot uu dot se changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                 CC|                            |mikpe at it dot uu dot se
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
> You are the assignee for the bug, or are watching the assignee.
>


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[mikpe at it dot uu dot se]

------- Comment #17 from mikpe at it dot uu dot se  2010-01-15 21:42 -------
(In reply to comment #16)
> --- testsuite/gcc.c-torture/execute/pr42691.c   (revision 155944)
> +++ testsuite/gcc.c-torture/execute/pr42691.c   (working copy)
> @@ -26,7 +26,8 @@
> 
>      i++;
>    }
> -  *deletedEntry = 0.0;
> +  if (deletedEntry)
> +    *deletedEntry = 0.0;
>    return 0;
>  }

Thanks, with this fix the test case exits normally with -O0 on both arm and
i686, but abort()s as intended with -Os -mthumb with 4.4 (pre-fix) on arm.

BTW, gcc-4.3.4 is also broken but gcc-4.2.4 works.


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]



------- Comment #18 from jingyu at google dot com  2010-01-15 21:46 -------
Subject: Re:  [4.4/4.5 regression] problematic 
        REG_EQUAL note added to SUBREG

On Fri, Jan 15, 2010 at 1:42 PM, mikpe at it dot uu dot se
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #17 from mikpe at it dot uu dot se  2010-01-15 21:42 -------
> (In reply to comment #16)
>> --- testsuite/gcc.c-torture/execute/pr42691.c   (revision 155944)
>> +++ testsuite/gcc.c-torture/execute/pr42691.c   (working copy)
>> @@ -26,7 +26,8 @@
>>
>>      i++;
>>    }
>> -  *deletedEntry = 0.0;
>> +  if (deletedEntry)
>> +    *deletedEntry = 0.0;
>>    return 0;
>>  }
>
> Thanks, with this fix the test case exits normally with -O0 on both arm and
> i686, but abort()s as intended with -Os -mthumb with 4.4 (pre-fix) on arm.
>

Sorry for the inconvenience. I only tested "-mthumb", which happened to work.
I should have proofread the code carefully.

I have reverted the whole patch on mainline and 4.4
I will modify the testcase and send the whole patch again.


> BTW, gcc-4.3.4 is also broken but gcc-4.2.4 works.

Thanks for testing 4.3.4. I don't have a working 4.3.x on hands.
I tested gcc-4.2.1, which works. So this is a regression bug.


>
>
> --
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
> You are the assignee for the bug, or are watching the assignee.
>


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[ebotcazou at gcc dot gnu dot org]

------- Comment #19 from ebotcazou at gcc dot gnu dot org  2010-01-15 21:50 -------
> I have reverted the whole patch on mainline and 4.4

That was unnecessary.

> I will modify the testcase and send the whole patch again.

Just send a message to gcc-patches@ and apply the fix to the testcase.


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at gcc dot gnu dot org]

------- Comment #20 from jingyu at gcc dot gnu dot org  2010-01-15 21:54 -------
Subject: Bug 42691

Author: jingyu
Date: Fri Jan 15 21:54:01 2010
New Revision: 155948

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155948
Log:
2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * combine.c (try_combine): Set changed_i3_dest to 1 when I2 and I3 set
        a pseudo to a constant and are merged, and adjust comments.

2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * gcc.c-torture/execute/pr42691.c: New.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr42691.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jakub at gcc dot gnu dot org]

------- Comment #21 from jakub at gcc dot gnu dot org  2010-01-15 21:59 -------
FYI, if you always want to test a NaN, you'd need to use big endian ordering
for some arches, plus there are targets which don't support NaN at all or where
a double isn't as big as 4 shorts.


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at gcc dot gnu dot org]

------- Comment #22 from jingyu at gcc dot gnu dot org  2010-01-15 22:11 -------
Subject: Bug 42691

Author: jingyu
Date: Fri Jan 15 22:11:43 2010
New Revision: 155949

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=155949
Log:
Backport from mainline(4.5) to fix a regression bug.

2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * combine.c (try_combine): Set changed_i3_dest to 1 when I2 and I3 set
        a pseudo to a constant and are merged, and adjust comments.

        2010-01-15  Jing Yu  <jingyu@google.com>

        PR rtl-optimization/42691
        * gcc.c-torture/execute/pr42691.c: New.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/gcc.c-torture/execute/pr42691.c
Modified:
    branches/gcc-4_4-branch/gcc/ChangeLog
    branches/gcc-4_4-branch/gcc/combine.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]



------- Comment #23 from jingyu at google dot com  2010-01-15 22:25 -------
Subject: Re:  [4.4/4.5 regression] problematic 
        REG_EQUAL note added to SUBREG

On Fri, Jan 15, 2010 at 1:59 PM, jakub at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #21 from jakub at gcc dot gnu dot org  2010-01-15 21:59 -------
> FYI, if you always want to test a NaN, you'd need to use big endian ordering
> for some arches,

If the floating point format is compatible with IEEE754,
on little-endian machine, double float 0x7ff0,0000,0000,0000 means an
infinite number.
on big-endian machine, double float 0x7ff0,0000,0000,0000 means a very
small number.
Then this test does not have problem.

>plus there are targets which don't support NaN at all or where
> a double isn't as big as 4 shorts.

This is troublesome.
We need to limit the test case to run on machines where
sizeof(double)=4*sizeof(short) only.
Any idea how to add conditions to the test case?

Thanks for pointing it out!

>
>
> --
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42691
>
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
> You are the assignee for the bug, or are watching the assignee.
>


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jakub at gcc dot gnu dot org]

------- Comment #24 from jakub at gcc dot gnu dot org  2010-01-16 16:05 -------
In the preprocessor you can do #if __SIZEOF_DOUBLE__ == 4 * __SIZEOF_SHORT__
(and in #else just have a int main (void) { return 0; }).
Or you could in main do if (sizeof (double) != 4 * sizeof (short)) return 0;


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[rguenth at gcc dot gnu dot org]

------- Comment #25 from rguenth at gcc dot gnu dot org  2010-01-20 23:30 -------
Please close this bug if it is fixed.


-- 


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

[Bug rtl-optimization/42691] [4.4/4.5 regression] problematic REG_EQUAL note added to SUBREG

[jingyu at google dot com]

------- Comment #26 from jingyu at google dot com  2010-01-20 23:48 -------
This bug is fixed in both trunk(4.5) and 4.4.
I will send a message to gcc-patches when I patch the test case.


-- 

jingyu at google dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


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