[Bug target/25268] New: ICE on lshrdi3_31 pattern

[Bug target/25268] New: ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

long long
foo (long long x, int y)
{
  unsigned long long x0 = (unsigned long long) x, x1;
  long long a = x0 >> ((y - 4) & 63);
  if ((a & 8) == 0)
    y--;
  x1 = x << ((24 - y) & 63);
  return (x1 >> 1);
}

ICEs at -m31 {-O1,-O2,-O3} on s390-linux.


-- 
           Summary: ICE on lshrdi3_31 pattern
           Product: gcc
           Version: 4.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jakub at gcc dot gnu dot org
GCC target triplet: s390-linux


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #1 from krebbel at gcc dot gnu dot org  2005-12-06 14:33 -------
Shorter testcase:

long long
foo (long long x, int y)
{
  return x << ((y + 1) & 63);
}

The count operand of a shift instruction is a somewhat weird
thing on S/390. It is basically an address accepting the sum of
an address register and a constant. To the resulting value an
AND mask of 63 is applied.

Looking for an easy way to get rid of pointless ANDs on a shift count
operand we've defined an 'Y' address constraint and a predicate
to accept these ANDs although our legitimize_address_p does not!
We were aware that this could potentially cause trouble but we found 
it working because find_reloads_address strips ANDs before handling the
address parts. 

Unfortunately find_reloads_address fails if there is actually a constant
addend present. If legitimize_address_p said no to an address consisting
of a register + constant it is assumed that the constant value is not
valid as displacement and is reloaded into a register if the machine
supports reg + reg addresses.

So we end up with:
r1 = c;
r3 << (r1 + r2); not a valid shift count operand

The only way to fix this seems to be making the 'Y' constraint and the 
shift_count_operand predicate to reject ANDs again and duplicating the shift
patterns in order to have a variant including the AND in the insn pattern.

I will come up with a patch as soon as possible.


-- 

krebbel 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         |2005-12-06 14:33:40
               date|                            |


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2005-12-06 14:48 -------
Yeah, making separate
(define_insn "*<shift>di3_and_31"
  [(set (match_operand:DI 0 "register_operand" "=d")
        (SHIFT:DI (match_operand:DI 1 "register_operand" "0")
                  (and:SI (match_operand:SI 2 "shift_count_operand" "Y")
                          (const_int 63)))]
  "!TARGET_64BIT"
  "s<lr>dl\t%0,%Y2"
  [(set_attr "op_type"  "RS")
   (set_attr "atype"    "reg")])

and 'Y'/shift_count_operand reject AND sounds best to me.  You don't need to
duplicate define_expand's I guess, just let combiner combine it.
It is not very nice to duplicate 14 patterns, on the other side lying to
reload that shift count is an address constraint when it is not is worse.


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

------- Comment #3 from jakub at gcc dot gnu dot org  2005-12-07 08:18 -------
BTW, is the s390 hw really masking the shift count with 63 for all insns,
or just the DImode shifts and with 31 for SImode shifts?
I'd say targets masking the shift count to number of bits of *shift<mode>3's
mode is very common on many targets, perhaps it would be better not to have
a special case in s390{,x} backend, but instead create a target macro
(or struct gcc_target's bool field) that would say if ASHIFT, LSHIFTRT,
ASHIFTRT,
ROTATE and ROTATERT mask the shift count to the number of bits in the shift
mode
(well, for ROTATE and ROTATERT I think that's true on all targets).
Then if that target macro is true, simplify_rtx could simplify
(*shift*:<mode> (reg:<mode>) (and:<mode2> (reg:<mode2>) (const_int C)))
into
(*shift*:<mode> (reg:<mode>) (reg:<mode2>)) if
(INTVAL (c) & (GET_MODE_BITSIZE (mode) - 1)) == GET_MODE_BITSIZE (mode) - 1)

Any ideas on what targets does say
(set (reg X) (const_int 65))
...
(set (reg:DI Y) (ashift:DI (reg:DI Y) (reg X)))
result in Y << 1 rather than in Y = 0?


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rth at gcc dot gnu dot org


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #4 from krebbel at gcc dot gnu dot org  2005-12-07 11:26 -------
(In reply to comment #3)
> BTW, is the s390 hw really masking the shift count with 63 for all insns,
> or just the DImode shifts and with 31 for SImode shifts?
On S/390 all shift count operands are masked with 63. SImode
as well as DImode shifts.

There are actually two target macros trying to address this issue:
SHIFT_COUNT_TRUNCATED and TARGET_SHIFT_TRUNCATION_MASK.

If you set SHIFT_COUNT_TRUNCATED to 1 the default used as truncation mask will
be GET_MODE_BITSIZE (mode) - 1 which is what you proposed. In the S/390 case 
we need a truncation mask of always 63 what I tried to introduce with this
patch:
http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00207.html

But this unfortunately doesn't help to get rid of pointless ANDs because
TARGET_SHIFT_TRUNCATION_MASK is defined to be valid only for SHIFTs coming
from named patterns. That makes it unusable for combine because it can't 
determine whether a SHIFT was generated due to a middle end optimization or
due to a named pattern.
For more information see (especially the mail from Richard Sandiford):
http://gcc.gnu.org/ml/gcc/2005-04/msg01066.html

If sometimes in the future all middle end optimizations touching SHIFTs will
be aware of the target macro we can remove the duplicated shift pattern I'm
about to introduce in the S/390 back end.


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

------- Comment #5 from jakub at gcc dot gnu dot org  2005-12-07 12:06 -------
Oops, sorry for not RTFSing before posting the comment here.
All I did was verify that x86_64 doesn't remove useless ANDs.
Should we add new peepholes or insns patterns to i386 for this too?


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

------- Comment #6 from jakub at gcc dot gnu dot org  2005-12-07 15:32 -------
Created an attachment (id=10434)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10434&action=view)
gcc41-s390-shift-and.patch

How does this patch look like then?  Sorry for not waiting for your patch,
but I need to do a build today and this is a show stopper for that build.


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #7 from krebbel at gcc dot gnu dot org  2005-12-07 16:26 -------
Created an attachment (id=10435)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10435&action=view)
Patch without testcase

I've bootstrapped the attached patch on s390 and s390x without 
testsuite regressions.
The patch matches almost exactly what you did. Additionally
I've merged the predicates setmem_operand and shift_count_operand
because after that change they contain the same code.
And the setmem_long pattern should as well be duplicated checking
for a mask of 255.
As testcase I think the smaller testcase I posted would be preferable.


-- 

krebbel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |krebbel at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[jakub at gcc dot gnu dot org]

------- Comment #8 from jakub at gcc dot gnu dot org  2005-12-07 16:52 -------
Created an attachment (id=10437)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=10437&action=view)
gcc41-pr25268.patch

Ok (not sure if it really is a good idea to make the *_operand names that
long), just I'm afraid ashrdi3_cc_64_and, ashrdi3_cc_31_and and ashrsi3_cc_and
will never match in your patch, because you haven't replaced (match_dup 2)
with (and:SI (match_dup 2) (match_dup 3)).
Attaching updated version of your patch with this and testcase incorporated.


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #9 from krebbel at gcc dot gnu dot org  2005-12-07 17:33 -------
(In reply to comment #8)
> Ok (not sure if it really is a good idea to make the *_operand names that
> long), 
Mmmh you are right but I couldn't think of a better name that moment.

just I'm afraid ashrdi3_cc_64_and, ashrdi3_cc_31_and and ashrsi3_cc_and
> will never match in your patch, because you haven't replaced (match_dup 2)
> with (and:SI (match_dup 2) (match_dup 3)).
Oops. Right - thanks for fixing this.

> Attaching updated version of your patch with this and testcase incorporated.
Thanks I've posted your updated version on gcc-patches and added your name to
the changelog entry.


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #10 from krebbel at gcc dot gnu dot org  2005-12-08 08:32 -------
Subject: Bug 25268

Author: krebbel
Date: Thu Dec  8 08:32:34 2005
New Revision: 108216

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=108216
Log:
2005-12-08  Andreas Krebbel  <krebbel1@de.ibm.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR target/25268
        * config/s390/s390.c (s390_decompose_shift_count): Remove BITS
        argument.  Don't drop outer ANDs.
        (s390_extra_constraint_str, print_shift_count_operand): Adjust callers.
        * config/s390/s390-protos.h (s390_decompose_shift_count): Adjust
        prototype.
        * config/s390/predicates.md (setmem_operand): Remove.
        (shift_count_operand): Rename to...
        (shift_count_or_setmem_operand): ... this.  Adjust
        s390_decompose_shift_count caller.
        * config/s390/s390.md (<shift>di3_31_and, <shift>di3_64_and,
        ashrdi3_cc_31_and, ashrdi3_cconly_31_and, ashrdi3_31_and,
        ashrdi3_cc_64_and, ashrdi3_cconly_64_and, ashrdi3_64_and,
        <shift>si3_and, ashrsi3_cc_and, ashrsi3_cconly_and, ashrsi3_and,
        rotl<mode>3_and, setmem_long_and): New insns.
        (<shift>di3_31, <shift>di3_64, ashrdi3_cc_31, ashrdi3_cconly_31,
        ashrdi3_31, ashrdi3_cc_64, ashrdi3_cconly_64, ashrdi3_64,
        <shift>si3, ashrsi3_cc, ashrsi3_cconly, ashrsi3, rotl<mode>3,
        <shift>di3, ashrdi3): Use shift_count_or_setmem_operand instead
        of shift_count_operand.
        (setmem_long): Use shift_count_or_setmem_operand instead of
        setmem_operand.

2005-12-08  Andreas Krebbel  <krebbel1@de.ibm.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR target/25268
        * gcc.c-torture/compile/20051207-1.c: New test.


Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/config/s390/predicates.md
    branches/gcc-4_1-branch/gcc/config/s390/s390-protos.h
    branches/gcc-4_1-branch/gcc/config/s390/s390.c
    branches/gcc-4_1-branch/gcc/config/s390/s390.md
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #11 from krebbel at gcc dot gnu dot org  2005-12-08 08:33 -------
Subject: Bug 25268

Author: krebbel
Date: Thu Dec  8 08:33:39 2005
New Revision: 108217

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=108217
Log:
2005-12-08  Andreas Krebbel  <krebbel1@de.ibm.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR target/25268
        * gcc.c-torture/compile/20051207-1.c: New test.


Added:
    branches/gcc-4_1-branch/gcc/testsuite/gcc.c-torture/compile/20051207-1.c


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[krebbel at gcc dot gnu dot org]

------- Comment #12 from krebbel at gcc dot gnu dot org  2005-12-08 08:56 -------
Subject: Bug 25268

Author: krebbel
Date: Thu Dec  8 08:56:24 2005
New Revision: 108220

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=108220
Log:
2005-12-08  Andreas Krebbel  <krebbel1@de.ibm.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR target/25268
        * config/s390/s390.c (s390_decompose_shift_count): Remove BITS
        argument.  Don't drop outer ANDs.
        (s390_extra_constraint_str, print_shift_count_operand): Adjust callers.
        * config/s390/s390-protos.h (s390_decompose_shift_count): Adjust
        prototype.
        * config/s390/predicates.md (setmem_operand): Remove.
        (shift_count_operand): Rename to...
        (shift_count_or_setmem_operand): ... this.  Adjust
        s390_decompose_shift_count caller.
        * config/s390/s390.md (<shift>di3_31_and, <shift>di3_64_and,
        ashrdi3_cc_31_and, ashrdi3_cconly_31_and, ashrdi3_31_and,
        ashrdi3_cc_64_and, ashrdi3_cconly_64_and, ashrdi3_64_and,
        <shift>si3_and, ashrsi3_cc_and, ashrsi3_cconly_and, ashrsi3_and,
        rotl<mode>3_and, setmem_long_and): New insns.
        (<shift>di3_31, <shift>di3_64, ashrdi3_cc_31, ashrdi3_cconly_31,
        ashrdi3_31, ashrdi3_cc_64, ashrdi3_cconly_64, ashrdi3_64,
        <shift>si3, ashrsi3_cc, ashrsi3_cconly, ashrsi3, rotl<mode>3,
        <shift>di3, ashrdi3): Use shift_count_or_setmem_operand instead
        of shift_count_operand.
        (setmem_long): Use shift_count_or_setmem_operand instead of
        setmem_operand.

2005-12-08  Andreas Krebbel  <krebbel1@de.ibm.com>
            Jakub Jelinek  <jakub@redhat.com>

        PR target/25268
        * gcc.c-torture/compile/20051207-1.c: New test.



Added:
    trunk/gcc/testsuite/gcc.c-torture/compile/20051207-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/s390/predicates.md
    trunk/gcc/config/s390/s390-protos.h
    trunk/gcc/config/s390/s390.c
    trunk/gcc/config/s390/s390.md
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug target/25268] ICE on lshrdi3_31 pattern

[pinskia at gcc dot gnu dot org]

------- Comment #13 from pinskia at gcc dot gnu dot org  2005-12-12 20:47 -------
Fixed.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.1.0


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