[Bug rtl-optimization/60763] New: ICE in extract_insn starting with rev 208984

[Bug rtl-optimization/60763] New: ICE in extract_insn starting with rev 208984

[pthaugen at gcc dot gnu.org]

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

            Bug ID: 60763
           Summary: ICE in extract_insn starting with rev 208984
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pthaugen at gcc dot gnu.org
                CC: bergner at gcc dot gnu.org, dje at gcc dot gnu.org,
                    rsandifo at gcc dot gnu.org
              Host: powerpc64-linux
            Target: powerpc64-linux
             Build: powerpc64-linux

Bootstrap build on PowerPC using --with-cpu=power8 started failing in stage2
build with stated revision. Also noticed with non-bootstrap build that several
testcases fail in similar manner. Following is example (reduced from
gcc.c-torture/compile/20020604-1.c).

[pthaugen@igoo delta]$ cat 20020604-1.c
foo (unsigned int n, int x, int y, unsigned char *z)
{
  {
    unsigned int i;
    for (i = 0; i < n; i++)
      {
    {
      union
      {
        float r;
        unsigned int i;
      }
      e;
      ((e.i >= 0x3f7f0000) ? ((int) e.i <
                  0) ? (unsigned char) 0 : (unsigned char) 255
       : (e.r = e.r * (255.0F / 256.0F) + 32768.0F, (unsigned char) e.i));
    }
      }
  }
}
[pthaugen@igoo delta]$ ~/install/gcc/trunk_work/bin/gcc -c -O1 -mcpu=power8
20020604-1.c
20020604-1.c: In function ‘foo’:
20020604-1.c:20:1: error: unrecognizable insn:
 }
 ^
(insn 64 63 65 5 (set (subreg:DI (reg:SF 32 0) 0)
        (reg:DI 8 8)) 20020604-1.c:16 -1
     (nil))
20020604-1.c:20:1: internal compiler error: in extract_insn, at recog.c:2202
0x105fafe3 _fatal_insn(char const*, rtx_def const*, char const*, int, char
const*)
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/rtl-error.c:109
0x105fb053 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/rtl-error.c:117
0x105c5723 extract_insn(rtx_def*)
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:2202
0x105c57ef extract_insn_cached(rtx_def*)
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:2105
0x10370f37 cleanup_subreg_operands(rtx_def*)
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/final.c:3063
0x105c46b7 split_insn
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:2920
0x105c498b split_all_insns()
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:2974
0x105c4b0f rest_of_handle_split_after_reload
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:3923
0x105c4b0f execute
    /home/pthaugen/src/gcc/trunk_work/gcc/gcc/recog.c:3952
Please submit a full bug report,
>From gcc-bugs-return-448318-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Fri Apr 04 16:30:00 2014
Return-Path: <gcc-bugs-return-448318-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 9236 invoked by alias); 4 Apr 2014 16:30:00 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 9190 invoked by uid 48); 4 Apr 2014 16:29:57 -0000
From: "bernd.edlinger at hotmail dot de" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug bootstrap/60743] build/genautomata uses 700 MB memory for ARM
Date: Fri, 04 Apr 2014 16:30:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: bootstrap
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: bernd.edlinger at hotmail dot de
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-60743-4-YzG5gZAzqW@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-60743-4@http.gcc.gnu.org/bugzilla/>
References: <bug-60743-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-04/txt/msg00338.txt.bz2
Content-length: 186

http://gcc.gnu.org/bugzilla/show_bug.cgi?id`743

--- Comment #9 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
The patch works for me.

Thanks for fixing this so quickly!

[Bug rtl-optimization/60763] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #1 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
I can't reproduce this locally.  Do I need any special configure flags
apart from -mcpu=power8?

It'd be interesting to see the insn that split_insn is splitting.
Hopefully that should give an idea which define_split is being used.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ice-on-valid-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-04-06
     Ever confirmed|0                           |1

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed due to the dup bug.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schwab@linux-m68k.org

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 60772 has been marked as a duplicate of this bug. ***

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Created attachment 32553
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32553&action=edit
Patch for 60772 testcase

OK, I could reproduce it with Andreas's instructions after configuring
with a real powerpc64-linux-gnu assembler.  I think the lack of an
assembler was causing some important configure test to fail.

One fix for the PR60772 case is attached.  It takes the same approach
as rs6000_split_multireg_move and IMO is preferable to allowing the
subreg in the movdi pattern.  The output is the same as before the
general_operand change.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[dje at gcc dot gnu.org]

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

--- Comment #5 from David Edelsohn <dje at gcc dot gnu.org> ---
I can see why the proposed patch will work, but it seems a little heavy-handed.
This case isn't something that simplify_gen_subreg() should handle?

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to David Edelsohn from comment #5)
> I can see why the proposed patch will work, but it seems a little
> heavy-handed. This case isn't something that simplify_gen_subreg() should
> handle?

Normally it does, but in this case the backend uses CANNOT_CHANGE_MODE_CLASS
to stop it:

      if (from_size < 8 || to_size < 8)
        return true;

But AIUI the mode change is OK in this split.  We start out with a 64-bit
value in which the upper 32 bits are significant, then convert it to SFmode.

Maybe it'd be more obvious if the input to vsx_xscvspdpn_directmove had
the DImode version of the register too, to emphasise that no mode change
happens outside the patterns.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32553|0                           |1
        is obsolete|                            |

--- Comment #7 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Created attachment 32557
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32557&action=edit
Updated patch that also uses op0_di for the conversion

Should be equivalent to the previous patch, but more correct modewise.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[meissner at gcc dot gnu.org]

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

Michael Meissner <meissner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |meissner at gcc dot gnu.org

--- Comment #8 from Michael Meissner <meissner at gcc dot gnu.org> ---
Richard's analysis agrees with mine.  The problem is with adding the check for
REG_CANNOT_CHANGE_MODE_P, it breaks the direct moves between GPRs and VSX
registers for SF.  Because internally within the PowerPC, SFmode is stored as
DFmode, CANNOT_CHANGE_MODE_CLASS returns true.  The original direct mvoe code
should not have generated a (SUBREG:DI (REG:SF ...)) in this case, since that
is illegal.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[pthaugen at gcc dot gnu.org]

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

--- Comment #9 from Pat Haugen <pthaugen at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #7)
> Created attachment 32557 [details]
> Updated patch that also uses op0_di for the conversion
> 
> Should be equivalent to the previous patch, but more correct modewise.

I tried this patch on a --with-cpu=power8 bootstrap and it now builds fine.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[dje at gcc dot gnu.org]

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

--- Comment #10 from David Edelsohn <dje at gcc dot gnu.org> ---
I'm not questioning the analysis, I'm questioning the solution. Directly
generating a register and jamming in the REGNO in this pattern seems sort of
crude.

gen_rtx_REG (DImode, REGNO (op0));

I don't doubt that this is the RTL that eventually is produced, I am surprised
that there is no existing wrapper function in simplify-rtx.c or rtlanal.c that
can be used.

If this is the necessary implementation, I think it deserves some, brief
comment.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #11 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to David Edelsohn from comment #10)
> I'm not questioning the analysis, I'm questioning the solution. Directly
> generating a register and jamming in the REGNO in this pattern seems sort of
> crude.
> 
> gen_rtx_REG (DImode, REGNO (op0));
> 
> I don't doubt that this is the RTL that eventually is produced, I am
> surprised that there is no existing wrapper function in simplify-rtx.c or
> rtlanal.c that can be used.
> 
> If this is the necessary implementation, I think it deserves some, brief
> comment.

I think the reason there's no wrapper function is that it's very dangerous
to change the mode of a register when the target has explicitly said that
that shouldn't happen.

Maybe it's an easier sell if I say that this op0_di is really a secondary
temporary value that just happens to use the same register as the output.
I think:

(define_insn_and_split "reload_vsx_from_gprsf"
  [(set (match_operand:SF 0 "register_operand" "=wa")
    (unspec:SF [(match_operand:SF 1 "register_operand" "r")]
           UNSPEC_P8V_RELOAD_FROM_GPR))
   (clobber (match_operand:DI 2 "register_operand" "=r"))
   (clobber (match_operand:DI 3 "register_operand" "=wa"))]
  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE"
  "#"
  "&& reload_completed"
  [(const_int 0)]
{
  rtx op0 = operands[0];
  rtx op1 = operands[1];
  rtx op2 = operands[2];
  rtx op3 = operands[3];
  rtx op1_di = simplify_gen_subreg (DImode, op1, SFmode, 0);

  /* Move SF value to upper 32-bits for xscvspdpn.  */
  emit_insn (gen_ashldi3 (op2, op1_di, GEN_INT (32)));
  emit_move_insn (op3, op2);
  emit_insn (gen_vsx_xscvspdpn_directmove (op0, op3));
  DONE;
}
  [(set_attr "length" "8")
   (set_attr "type" "two")])

would work too.  The difference is that the above would always allocate
two separate FPRs, which is wasteful, so the pattern is trying to use
the same register for both the temporary value and the final result.

But I suppose it just doesn't seem that crude to me.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[dje at gcc dot gnu.org]

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

--- Comment #13 from David Edelsohn <dje at gcc dot gnu.org> ---
Gentlemen, I really do not understand this discussion.

I used the term "crude" and receive a response that it is not crude, but it is
dangerous. WTF? Why is anyone trying to "sell" the patch when I repeatedly have
said that I do not disagree with it in principle? I only am trying to ensure
that this is the right use of the GCC API. i386 uses this idiom a lot; rs6000
does not.

I see multiple responses trying to convince me of a patch with which I already
agree and no one writing a one line comment.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32557|0                           |1
        is obsolete|                            |

--- Comment #15 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Created attachment 32568
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32568&action=edit
Patch with changelog and comment.

Here's a version of the patch with a comment explaining the gen_rtx_REG.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[dje at gcc dot gnu.org]

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

--- Comment #16 from David Edelsohn <dje at gcc dot gnu.org> ---
Comment on attachment 32568
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32568
Patch with changelog and comment.

Okay. Thanks for the clarification.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #17 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
I'm just saying this for the record, and also to justify why I think
the other use of simplify_gen_subreg is correct.

(In reply to Michael Meissner from comment #14)
> When you are doing a subreg type operation, before reload, you must NOT use the
> register number directly.  This is the dangerous part that was mentioned.

David was quoting me with the dangerous thing, so for what it's worth,
I meant that it would be dangerous to have a version of simplify_gen_subreg
that ignored CANNOT_CHANGE_MODE_CLASS.  And as long as simplify_gen_subreg
honours CANNOT_CHANGE_MODE_CLASS, a DImode subreg of an SFmode FPR is going
to return a subreg rather than a plain reg.

I definitely agree that using gen_rtx_REG before reload is dangerous
too of course.  I just wanted to clarify what I meant.

> Hence using gen_highpart, gen_lowpart, or simplify_gen_subreg is the
> appropriate solution.

Agreed,

> If you are doing splits after reload, and are dealing with whole registers, the
> preferred solution is to create a new REG with the appropriate register number.
> In particular, gen_highpart and gen_lowpart must not be used after reload.
> However, up until the patch to add more checking, simplify_gen_subreg could be
> used after reload.

FWIW, I think gen_lowpart, gen_highpart and simplify_gen_subreg are always
OK if what you are doing is changing the mode of a value.  They are supposed
to handle both hard and pseudo registers.  So I think the other use of
simplify_gen_subreg in this pattern is OK (and gen_lowpart would be have
been OK too).  Using gen_rtx_REG wouldn't be wrong as such, but it would
lose things like ORIGINAL_REGNO.

The difference here is that we have a DImode temporary value and an SFmode
result that both need the same hard register.  The DImode value is written
and read only in DImode and the result is written only in SFmode, so there's
no real mode change.  That's the kind of case where gen_rtx_REG is needed.

> Out of force of habit, I've tended to use simplify_gen_subreg when doing
> splitting, no matter whether it is before or after reload.  I thought with
> 'simplify' in the name, it would do the right thing after reload, but evidently
> it does not.

Well, I suppose it depends on what the expectations are.  If the target
forbids a particular mode change for a particular hard register,
simplify_gen_subreg will return a subreg rather than a reg.  IMO that's
the right thing to do, since it keeps the information that you have
a mode change that can't be reduced to a plain reg.  Returning null
would be another option (maybe better) but would probably have a big
impact.

So I think the problem in this case was that we were modelling the reuse
of the destination register as a mode change when it wasn't really,
and the mode change in question was one that the target normally tries
to prevent.

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[rsandifo at gcc dot gnu.org]

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

--- Comment #18 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Author: rsandifo
Date: Tue Apr  8 17:50:45 2014
New Revision: 209223

URL: http://gcc.gnu.org/viewcvs?rev=209223&root=gcc&view=rev
Log:
gcc/
    PR target/60763
    * config/rs6000/vsx.md (vsx_xscvdpspn_scalar): Change input to DImode.
    * config/rs6000/rs6000.md (reload_vsx_from_gprsf): Update accordingly.
    Use gen_rtx_REG rather than simplify_gen_subreg for op0_di.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/rs6000.md
    trunk/gcc/config/rs6000/vsx.md

[Bug rtl-optimization/60763] [4.9 Regression] ICE in extract_insn starting with rev 208984

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.