[Bug regression/41188] New: move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[Bug regression/41188] New: move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

This bug was discovered while adapting MSP430 port for GCC 4.x and is
persistent at least in both GCC 4.2.4 and 4.3.4.
This description consists of a problem description, a detailed reason analysis
pointing out exact GCC functions, and a proposed solution.
---------------------------------------------------------------
1. Problem
Compiling the following code for MSP430 (16-bit RISC) with maximum optimization
(-O2) causes an "internal error int redirect_branch_edge()" failure:
-------------
for (volatile long t = 0; t < 1024; t++) ;
-------------
The problem is related to the "cbranchsi" instruction, that clobbers one of
compared registers. Here is a quote from MD file:
-------------
(define_insn "*cbranchsi_others"
  [(parallel [(set (pc)
     (if_then_else (match_operator:SI 1 "inequality_operator"
                    [(match_operand:SI 2 "register_operand" "r")
                     (match_operand:SI 3 "general_operand" "rmi")])
      (label_ref (match_operand 0 "" ""))
      (pc)))
  (clobber (match_dup 2))])] ;<========== Attention! clobber (match_dup) !!!
-------------
2. Reason
2.1 The "*cbranchsi_others" INSN clobbers one of its arguments [clobber
(match_dup 2)]. During loop optimization, the source register (operand 2) gets
changed, while the register reference in CLOBBER attribute does not. After such
change, instruction is no longer recognizable according to machine decsription,
and the compilation fails.
2.2 The move_invariant_reg() function in loop-invariant.c performs register
renaming based on "register use lists". When the "cbranchsi_others" instruction
gets scanned for register uses, the CLOBBER use does not get recorded, and
thus, does not get changed later, resulting in an erroneous INSN pattern:
------------- Before move_invariant_reg() (insn_invalid_p(insn) = 0)
-------------
(parallel [
        (set (pc)
            (if_then_else (ge:SI (reg:SI 30)
                    (reg:SI 24 [ j.1 ]))
                (label_ref:HI 34)
                (pc)))
        (clobber:SI (reg:SI 30))
    ])
------------- After move_invariant_reg() (insn_invalid_p(insn) = 0)
-------------
(parallel [
        (set (pc)
            (if_then_else (ge:SI (reg:SI31) <===== CHANGED 30 to 31
                    (reg:SI 24 [ j.1 ]))
                (label_ref:HI 34)
                (pc)))
        (clobber:SI (reg:SI 30))  <======= NOT CHANGED 30 TO 31
    ])
-------------
Obviously, the modification makes the instruction invalid, as it uses and
clobbers different registers, and thus, no existing INSN pattern matches it.
2.3 The "register use lists" are built using the record_uses() function in
loop-invariant.c based on DF use records. The record_uses() function considers
DF_INSN_USES() and DF_INSN_EQ_USES() lists (only DF_INSN_USES in gcc 4.2.4).
CLOBBERed registers are put into DF_INSN_DEFS() list, that is not considered in
record_uses(). That way, a CLOBBER attribute remains unchanged, when used
registers are renamed.
2.4 The df_defs_record() function in df-scan.c places both SET and CLOBBER
references into DF_INSN_DEFS() list:
  if (code == SET || code == CLOBBER)
    {
      //...
      df_def_record_1 (collection_rec, x, bb, insn, clobber_flags);
    }
2.5 Summary. Invariant moving optimization in loop-invariant.c disrupts INSN
patterns that USE and CLOBBER the same register, making them invalid.
-------------
3. Proposed solution.
3.1. For my needs, I have patched loop-invariant.c by adding the following
function to explicitly record all CLOBBER references from INSN that match USE
references:
static void record_clobber_uses_workaround(rtx insn, struct invariant *inv,
struct df_ref *use)
{
        rtx pat = PATTERN(insn);
        if (GET_CODE(pat) == PARALLEL)
        {
                int len = XVECLEN(pat, 0);
                int idx = 0;

                for (idx = 0; idx < len; idx++)
                {
                        rtx subexp = XVECEXP(pat, 0, idx);
                        if (GET_CODE(subexp) == CLOBBER)
                        {
                                if (XEXP (subexp, 0) == *DF_REF_REAL_LOC (use))
                                        record_use (inv->def, &XEXP (subexp,
0), DF_REF_INSN (use));
                        }                               
                }
        }
}
The record_uses() function got patched accordingly:
 if (inv)
+  {
        record_use (inv->def, DF_REF_REAL_LOC (use), DF_REF_INSN (use));
+       record_clobber_uses_workaround(insn, inv, use);         
+  }
3.2. I would recommend re-analyzing the more common case, when a single
instruction uses and defines the same register, and decide, whether loop
optimizations in loop-invariant.c should move such register. The bug does not
seem to affect most architectures, however, specific INSN patterns, as
described above, do trigger it. Maybe, a common function returning the list of
all "match_dup"-ed references for a given INSN and an operand to detect such
cases in a uniform way.


-- 
           Summary: move_invariant_reg() damages CBRANCH instructions with
                    CLOBBER attribute
           Product: gcc
           Version: 4.3.4
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: regression
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: shcherbakov at daad-alumni dot de
 GCC build triplet: independent (i686-cygwin)
  GCC host triplet: independent (i686-cygwin)
GCC target triplet: msp430 (see below)


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

------- Comment #1 from shcherbakov at daad-alumni dot de  2009-08-30 10:10 -------
A misprint in the description: 
After move_invariant_reg() the insn_invalid_p(insn) returns 1 meaning that the
INSN is no longer valid.


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

------- Comment #2 from shcherbakov at daad-alumni dot de  2009-08-30 10:13 -------
*** Bug 41191 has been marked as a duplicate of this bug. ***


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

------- Comment #3 from shcherbakov at daad-alumni dot de  2009-08-30 10:14 -------
*** Bug 41190 has been marked as a duplicate of this bug. ***


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

------- Comment #4 from shcherbakov at daad-alumni dot de  2009-08-30 10:14 -------
*** Bug 41189 has been marked as a duplicate of this bug. ***


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[steven at gcc dot gnu dot org]

------- Comment #5 from steven at gcc dot gnu dot org  2009-08-31 06:20 -------
See http://gcc.gnu.org/ml/gcc-patches/2009-08/msg01291.html


-- 

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-08-31 06:20:14
               date|                            |


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[amylaar at gcc dot gnu dot org]

------- Comment #6 from amylaar at gcc dot gnu dot org  2009-09-02 12:18 -------
Note that I discovered this bug in the milepost code only after single-stepping
through the SIMD co-processor code to find out what was going wrong.
A match_dup is only effective when an instruction is recognized, and reload
only cares about all the constraints being satisfied.  Since the instruction
was recognized before move_invariant_reg, its insn_code was cached, and as far
as all the subsequent passes were concerned, there was no reason to
re-recognize the instruction which had the invalid change.

In other cases, e.g. when reload has to do some real work, you might see an
ICE.


-- 

amylaar at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  GCC build triplet|independent (i686-cygwin)   |independent (i686-cygwin) /
                   |                            |x86_64-unknown-linux-gnu
                   |                            |(gcc13@fsff
 GCC target triplet|msp430 (see below)          |msp430 (see below) / --
                   |                            |target=arc-elf32 --with-
                   |                            |extra-target-list
           Keywords|                            |ice-on-valid-code, wrong-
                   |                            |code


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[amylaar at gcc dot gnu dot org]

------- Comment #7 from amylaar at gcc dot gnu dot org  2009-09-02 12:27 -------
(In reply to comment #0)
> 3.1. For my needs, I have patched loop-invariant.c by adding the following
> function to explicitly record all CLOBBER references from INSN that match USE
> references:
> static void record_clobber_uses_workaround(rtx insn, struct invariant *inv,
> struct df_ref *use)
> {
>         rtx pat = PATTERN(insn);
>         if (GET_CODE(pat) == PARALLEL)
>         {
>                 int len = XVECLEN(pat, 0);
>                 int idx = 0;
> 
>                 for (idx = 0; idx < len; idx++)
>                 {
>                         rtx subexp = XVECEXP(pat, 0, idx);
>                         if (GET_CODE(subexp) == CLOBBER)
>                         {
>                                 if (XEXP (subexp, 0) == *DF_REF_REAL_LOC (use))
>                                         record_use (inv->def, &XEXP (subexp,
> 0), DF_REF_INSN (use));
>                         }                               
>                 }
>         }
> }
> The record_uses() function got patched accordingly:
>  if (inv)
> +  {
>         record_use (inv->def, DF_REF_REAL_LOC (use), DF_REF_INSN (use));
> +       record_clobber_uses_workaround(insn, inv, use);         
> +  }

This change is incorrect because a register ceases to be a loop invariant
if it is clobbered.

> 3.2. I would recommend re-analyzing the more common case, when a single
> instruction uses and defines the same register, and decide, whether loop
> optimizations in loop-invariant.c should move such register. The bug does not
> seem to affect most architectures, however, specific INSN patterns, as
> described above, do trigger it. Maybe, a common function returning the list of
> all "match_dup"-ed references for a given INSN and an operand to detect such
> cases in a uniform way.

That would not be general enough.  You could also have an insn predicate which
requires specific correlations and/or properties of the operands.

Using validate_change, as I did in my patch, also covers these contingencies.


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[amylaar at gcc dot gnu dot org]

------- Comment #8 from amylaar at gcc dot gnu dot org  2009-09-02 12:35 -------
Somehow bugzilla keeps eating some of the text I put in the Target: field.
My target settings were:
--target=arc-elf32 --with-extra-target-list='mxp-elf'


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[shcherbakov at daad-alumni dot de]

------- Comment #9 from shcherbakov at daad-alumni dot de  2009-09-02 16:12 -------
> This change is incorrect because a register ceases to be a loop invariant
> if it is clobbered.

Agreed. Tested your patch. Works fine for me.

Is there any way to know, from which version will the patch be included in gcc
releases?


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[amylaar at gcc dot gnu dot org]

------- Comment #10 from amylaar at gcc dot gnu dot org  2009-09-02 16:22 -------
(In reply to comment #9)
> Is there any way to know, from which version will the patch be included in gcc
> releases?

If a patch is installed with a suitable PR marker in the log entry, you will
get notification via this PR.
At any rate, this bug should not be closed till it is fixed in mainline, and
again, you will be notified when this PR changes state.  You can then check
http://gcc.gnu.org/develop.html#timeline what the current development plans
are.  Note, although patches generally have to be installed in mainline first,
that does not automatically mean that they will be installed in active release
branches; they have to be approved specifically for each branch before they
can be installed/backported there.
Not all patches that could be installed/backported in release branches
actually are, depending on what the priorities for the release and the
imact assessment of the patch is.


-- 


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

[Bug regression/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[amylaar at gcc dot gnu dot org]

------- Comment #11 from amylaar at gcc dot gnu dot org  2009-09-09 04:57 -------
Subject: Bug 41188

Author: amylaar
Date: Wed Sep  9 04:56:40 2009
New Revision: 151548

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=151548
Log:
merge selected patches from milepost-integration:
        * config/arc/arc.c (arc_expand_prologue): Cast pretend_size to
        HOST_WIDE_INT before negating.
        * config/arc/arc.md (umulsi3_highpart): Don't wrap a ZERO_EXTEND
        around a CONST_INT.

        PR regression/41188
        * loop-invariant.c (move_invariant_reg): Check changes for validity.

        * config/arc/arc.c (arc_conditional_register_usage): Fix
        regno_reg_class for ilink[12] on ARC700.

        * config/arc/arc.h (LARGE_INT): Fix 64-bit behaviour.

        * doc/tm.texi (TARGET_VALID_OPTION_ATTRIBUTE_P): Change to:
        (TARGET_OPTION_VALID_ATTRIBUTE_P).

Modified:
    branches/arc-4_4-20090909-branch/gcc/ChangeLog.ARC
    branches/arc-4_4-20090909-branch/gcc/config/arc/arc.c
    branches/arc-4_4-20090909-branch/gcc/config/arc/arc.h
    branches/arc-4_4-20090909-branch/gcc/config/arc/arc.md
    branches/arc-4_4-20090909-branch/gcc/doc/tm.texi
    branches/arc-4_4-20090909-branch/gcc/loop-invariant.c


-- 


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

[Bug rtl-optimization/41188] move_invariant_reg() damages CBRANCH instructions with CLOBBER attribute

[steven at gcc dot gnu dot org]

------- Comment #12 from steven at gcc dot gnu dot org  2010-07-20 22:50 -------
Someone should dust off the patch and submit it for trunk. Joern?


-- 

steven at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|regression                  |rtl-optimization
   Last reconfirmed|2009-08-31 06:20:14         |2010-07-20 22:50:10
               date|                            |


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