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

["jingyu at google dot com" <gcc-bugzilla at gcc dot gnu dot org>]

------- 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