Bug 99422

Seems the latest change resulted in various inline asms with "=m" constraint to fall through into return false rather than decompose_address.
The problem to me seems to be that constraint is "=m" (and generally, it is the whole constraint string, rather than some particular constraint in it),
and lookup_constraint looks up only a particular constraint (say "m") etc.
So it only returns something other than CONSTRAINT__UNKNOWN if it is an input operand rather than output (because that necessarily starts with = or +) and doesn't have any other modifiers etc. in it and it has a single variant or the first variant is the one that is considered.
There are other spots that do the same though, e.g. simplify_operand_subreg:
          || ((get_constraint_type (lookup_constraint
                                    (curr_static_id->operand[nop].constraint))
               != CT_SPECIAL_MEMORY)
While e.g. process_alt_operands walks the whole constraint string and handles everything in there.

I'm afraid I must be missing something important.

I see the function is called before selecting a particular alternative, so perhaps it means to care only about constraints like "X" and "" and not say that mixed with other constraints etc.
But, shouldn't the code at least skip the =, +, &, % and whitespace from the start?  What about other modifiers (the various disparage slightly etc. chars)?
And only consider as empty constraint if after those skips constraint is ""?
Not really sure if ",,," constraint is valid...
And, regarding of Eric's change to handle "X" that way, does that really apply just to MEM and not SUBREG of MEM too?

I also see this when compiling linux kernel 5.12-rc2.

net/netfilter/utils.c: In function ‘nf_checksum’:
net/netfilter/utils.c:139:1: error: unrecognizable insn:
  139 | }
      | ^
(insn 120 466 414 12 (parallel [
            (set (reg:SI 0 ax [orig:200 sum ] [200])
                (asm_operands:SI ("  addl %1, %0
  adcl %2, %0
  adcl %3, %0
  adcl $0, %0
") ("=r") 0 [
                        (mem/c:SI (plus:DI (reg/f:DI 7 sp)
                                (const_int 24 [0x18])) [301 %sfp+-8 S4 A64])
                        (mem:SI (plus:DI (mem/c:DI (plus:DI (reg/f:DI 7 sp)
                                        (const_int 16 [0x10])) [301 %sfp+-16 S8 A64])
                                (const_int 12 [0xc])) [1 MEM[(const struct iphdr *)_19].saddr+0 S4 A32])
                        (reg:SI 1 dx [203])
                        (reg:SI 0 ax [orig:200 sum ] [200])
                    ]
                     [
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("g") ./arch/x86/include/asm/checksum_64.h:91)
                        (asm_input:SI ("0") ./arch/x86/include/asm/checksum_64.h:91)
                    ]
                     [] ./arch/x86/include/asm/checksum_64.h:91))
            (clobber (reg:CC 17 flags))
        ]) "./arch/x86/include/asm/checksum_64.h":91:2 -1
     (nil))

*** Bug 99432 has been marked as a duplicate of this bug. ***

*** Bug 99438 has been marked as a duplicate of this bug. ***

(In reply to Joseph S. Myers from comment #0)
> Created attachment 50314 [details]
> preprocessed source
> 
> Commit 9105757a59b890194ebf5b4fcbacd58db34ef332 ("[PR99378] LRA: Skip
> decomposing address for asm insn operand with unknown constraint.")
> introduced the following ICE building glibc for i686-linux-gnu. Tested with
> an x86_64 compiler; compile the attached .i file with the following options:
> -m32 -march=i686 -O2 -pg -S
> 

Sorry for causing the troubles.  I'll fix this tomorrow.

(In reply to Jakub Jelinek from comment #2)
> I see the function is called before selecting a particular alternative, so
> perhaps it means to care only about constraints like "X" and "" and not say
> that mixed with other constraints etc.
> But, shouldn't the code at least skip the =, +, &, % and whitespace from the
> start?  What about other modifiers (the various disparage slightly etc.
> chars)?
> And only consider as empty constraint if after those skips constraint is ""?
> Not really sure if ",,," constraint is valid...
> And, regarding of Eric's change to handle "X" that way, does that really
> apply just to MEM and not SUBREG of MEM too?

Yes.  It seems my bad job on reviewing Richard Sandiford's patch 777e635f1a6c.  Before this patch constraint string was checked only for 'p' which can not have modifiers (although spaces are still possible).

I am afraid that fixing this mess can result in new failures.  But we should do this anyway.

*** Bug 99454 has been marked as a duplicate of this bug. ***

*** Bug 99455 has been marked as a duplicate of this bug. ***

The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

https://gcc.gnu.org/g:04b4828c6dd215385fde6964a5e13da8a01a78ba

commit r11-7554-g04b4828c6dd215385fde6964a5e13da8a01a78ba
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Mon Mar 8 09:24:57 2021 -0500

    [PR99422] LRA: Skip modifiers when processing memory address.
    
      Function process_address_1 can wrongly look at constraint modifiers
    instead of the 1st constraint itself.  The patch solves the problem.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (skip_contraint_modifiers): New function.
            (process_address_1): Use it before lookup_constraint call.

I think I fixed the PR.  Although there may be necessity for one more patch to solve other process_address_1 issues. I did not decide this yet.

Unfortunately, I'm still seeing the ICE reported in PR target/99432 on
i386-pc-solaris2.11.

*** Bug 99461 has been marked as a duplicate of this bug. ***

*** Bug 99467 has been marked as a duplicate of this bug. ***

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99438 still happens,
i.e. i586-unknown-freebsd11.2.

Is anyone able to successfully build any 32-bit x86 target?

the kernel build failures still happen. I am re-opened #99454 and #99455

> --- Comment #12 from Rainer Orth <ro at gcc dot gnu.org> ---
> Unfortunately, I'm still seeing the ICE reported in PR target/99432 on
> i386-pc-solaris2.11.

While I could build yesterday's tree with the PR 99378 patch included on
sparc-sun-solaris2.11, today with the PR 99422 patch in tree as well,
bootstrap is broken:

a-ztflau.adb: In function 'Ada.Float_Wide_Wide_Text_Io.Aux_Float.Puts':
a-ztflau.adb:132:8: error: insn does not satisfy its constraints:
(insn 174 39 180 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
                (const_int -5216 [0xffffffffffffeba0])) [37 %sfp+-5216 S8 A64])
        (reg:DF 40 %f8 [203])) "a-ztflau.adb":117:7 153 {*movdf_insn_sp32}
     (nil))
during RTL pass: reload
+===========================GNAT BUG DETECTED==============================+
| 11.0.1 20210308 (experimental) [master revision 0d9a70ea3881c284b7689b691d54d047b55b486d] (sparc-sun-solaris2.11) GCC error:|
| in extract_constrain_insn, at recog.c:2670                               |
| Error detected around a-ztflau.adb:132:8                 

Reverting both patches locally allowed both sparc-sun-solaris2.11 and
i386-pc-solaris2.11 bootstraps to succeed.

With the two patches for PR 99454, a i386-pc-solaris2.11 bootstraps succeeds.
Thanks.

Unfortunately, on the SPARC side I still get the ICE reported in Comment 17.

> a-ztflau.adb: In function 'Ada.Float_Wide_Wide_Text_Io.Aux_Float.Puts':
> a-ztflau.adb:132:8: error: insn does not satisfy its constraints:
> (insn 174 39 180 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
>                 (const_int -5216 [0xffffffffffffeba0])) [37 %sfp+-5216 S8
> A64])
>         (reg:DF 40 %f8 [203])) "a-ztflau.adb":117:7 153 {*movdf_insn_sp32}
>      (nil))

The offset is out of the allowed range [-4096, 4095] so something is broken.

I started to work on another, more safe approach to solve the problems.  I hope the patch will be ready today or tomorrow.

The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

https://gcc.gnu.org/g:d38bbb85117a9947797f10d459fe0c23ea479313

commit r11-7613-gd38bbb85117a9947797f10d459fe0c23ea479313
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Wed Mar 10 16:15:08 2021 -0500

    [PR99422] LRA: Don't check unknown constraint, use X for empty constraint
    
    Using CONSTRAINT__UNKNOWN was a bad idea, although it triggered a lot
    hidden bugs.  It is better to use X instead of empty constraint.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (process_address_1): Don't check unknown
            constraint, use X for empty constraint.

Could you check the patch on the failed bootstraps.  I have no access to solaris machines.
Thank you.

> --- Comment #22 from Vladimir Makarov <vmakarov at gcc dot gnu.org> ---
> Could you check the patch on the failed bootstraps.  I have no access to

Unfortunately, it didn't help.  I continue to revert the patches for PRs
99378, 99422, and 99454 locally for now.

> solaris machines.

Technically, you have ;-)  There's gcc211 in the cfarm, but
unfortunately the installation is highly nonstandard, doesn't follow
cfarm conventions, and lacks GNAT.  While I do have a local gcc
installation on the box without these issues, I agree it would be quite
tedious to use.  I suspect that the same issue occurs on Linux/sparc64,
too (gcc202), but trying that on a machine that's pretty unreliable
wouldn't be my idea of fun.

> Technically, you have ;-)  There's gcc211 in the cfarm, but
> unfortunately the installation is highly nonstandard, doesn't follow
> cfarm conventions, and lacks GNAT.  While I do have a local gcc
> installation on the box without these issues, I agree it would be quite
> tedious to use.  I suspect that the same issue occurs on Linux/sparc64,
> too (gcc202), but trying that on a machine that's pretty unreliable
> wouldn't be my idea of fun.

This can be reproduced with a minimal Ada cross-compiler, i.e. you just need the gnat1 compiler, the skeleton of libada and the command line:
  $(srcdir)/configure --target=sparc-sun-solaris2.11 --enable-languages=c,c++,ada
  make CFLAGS=-g CXXFLAGS=-g ADAFLAGS="-gnatpga -gnatws"
  cd gcc/ada; make gnatlib
  cd rts; ../../gnat1 -quiet a-lfztio.ads -gnatpg

a-ztflau.adb: In function 'Ada.Long_Float_Wide_Wide_Text_Io.Aux_Float.Put':
a-ztflau.adb:101:8: error: insn does not satisfy its constraints:
(insn 32 31 88 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
                (const_int -5232 [0xffffffffffffeb90])) [29 %sfp+-5232 S8 A64])
        (reg:DF 40 %f8 [orig:109 _1 ] [109])) "a-ztflau.adb":99:7 153 {*movdf_insn_sp32}
     (nil))
during RTL pass: reload
+===========================GNAT BUG DETECTED==============================+
| 11.0.1 20210310 (experimental) [master revision 1c3c12b0a6f:f62bd375583:47403a0eefac52636db768dc46c3c88a2cd4b28e] (sparc-sun-solaris2.11) GCC error:|
| in extract_constrain_insn, at recog.c:2670                               |
| Error detected around a-ztflau.adb:101:8

(In reply to Eric Botcazou from comment #24)
 
> This can be reproduced with a minimal Ada cross-compiler, i.e. you just need
> the gnat1 compiler, the skeleton of libada and the command line:
>   $(srcdir)/configure --target=sparc-sun-solaris2.11
> --enable-languages=c,c++,ada
>   make CFLAGS=-g CXXFLAGS=-g ADAFLAGS="-gnatpga -gnatws"
>   cd gcc/ada; make gnatlib
>   cd rts; ../../gnat1 -quiet a-lfztio.ads -gnatpg
> 
> a-ztflau.adb: In function 'Ada.Long_Float_Wide_Wide_Text_Io.Aux_Float.Put':
> a-ztflau.adb:101:8: error: insn does not satisfy its constraints:
> (insn 32 31 88 2 (set (mem/c:DF (plus:SI (reg/f:SI 30 %fp)
>                 (const_int -5232 [0xffffffffffffeb90])) [29 %sfp+-5232 S8
> A64])
>         (reg:DF 40 %f8 [orig:109 _1 ] [109])) "a-ztflau.adb":99:7 153
> {*movdf_insn_sp32}
>      (nil))
>

Thank you.  I reproduced it.
The patches probably triggered a hidden bug.
I'll investigate more and write my findings today.

Here are my findings.

Before the patches function process_address_1 used CONSTRAINT__UNKNOWN (taken from '=' of constraint "=T,..." and this is wrong) to check validity address.  It was invalid and LRA added reloads for the address.

After the patches, the function uses CONTSTRAINT_T (taken from 'T').  For constraint T sparc code says that the memory address is ok and LRA keeps the address and does not generate reloads.

That is wrong.  Sparc code should say LRA that the address is wrong.  Function sparc.c:memory_ok_for_ldd is responsible for this.

If I apply the following patch

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c                                                                                                                                                                                              
index f1504172022..ac83f900964 100644                                                                                                                                                                                                                         
--- a/gcc/config/sparc/sparc.c                                                                                                                                                                                                                                
+++ b/gcc/config/sparc/sparc.c                                                                                                                                                                                                                                
@@ -9230,6 +9230,9 @@ memory_ok_for_ldd (rtx op)                                                                                                                                                                                                              
   if (! can_create_pseudo_p ()                                                                                                                                                                                                                               
       && !strict_memory_address_p (Pmode, XEXP (op, 0)))                                                                                                                                                                                                     
     return 0;                                                                                                                                                                                                                                                
+  if (can_create_pseudo_p ()                                                                                                                                                                                                                                 
+      && !memory_address_p (Pmode, XEXP (op, 0)))                                                                                                                                                                                                            
+    return 0;                                                                                                                                                                                                                                                

   return 1;
 }

the problem is gone.

I think target code is responsible for the bug and fix should be there not in LRA.

> Before the patches function process_address_1 used CONSTRAINT__UNKNOWN
> (taken from '=' of constraint "=T,..." and this is wrong) to check validity
> address.  It was invalid and LRA added reloads for the address.

Is that because T is a special_memory_constraint or did it happen with a regular memory_constraint as well?

> After the patches, the function uses CONTSTRAINT_T (taken from 'T').  For
> constraint T sparc code says that the memory address is ok and LRA keeps the
> address and does not generate reloads.

So LRA does not check that the memory is valid for special_memory_constraint or memory_constraint, and the constraint must do it on its own?  Then it's probably inherited from reload and I guess that

  if (! can_create_pseudo_p ()
      && !strict_memory_address_p (Pmode, XEXP (op, 0)))
    return 0;

was supposed to do it, in which case David and I missed it when we converted the port to LRA.  This would point to:

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index f1504172022..d8860f908e9 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -9227,7 +9227,7 @@ memory_ok_for_ldd (rtx op)
   if (TARGET_ARCH32 && !mem_min_alignment (op, 8))
     return 0;
 
-  if (! can_create_pseudo_p ()
+  if ((lra_in_progress || reload_in_progress)
       && !strict_memory_address_p (Pmode, XEXP (op, 0)))
     return 0;

Thanks for the investigation, I'll take over from there.

The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:d8b84e2771fc2495493d0c66c3cba714484757d7

commit r11-7650-gd8b84e2771fc2495493d0c66c3cba714484757d7
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Fri Mar 12 17:07:20 2021 +0100

    Fix memory constraint bug in SPARC back-end
    
    It's a bug exposed by the recent LRA changes, whereby the T constraint
    fails to behave properly when LRA is enabled (unlike when reload is
    enabled).  The patch also gets rid of the awkward W constraint, which
    is strictly equivalent to m in 64-bit mode and, as a result, renames
    the w constraint into W.
    
    gcc/
            PR target/99422
            * config/sparc/constraints.md (w): Rename to...
            (W): ... this and ditch previous implementation.
            * config/sparc/sparc.md (*movdi_insn_sp64): Replace W with m.
            (*movdf_insn_sp64): Likewise.
            (*mov<VM64:mode>_insn_sp64): Likewise.
            * config/sparc/sync.md (*atomic_compare_and_swap<mode>_1): Replace
            w with W.
            (atomic_compare_and_swap_leon3_1): Likewise.
            (*atomic_compare_and_swapdi_v8plus): Likewise.
            * config/sparc/sparc.c (memory_ok_for_ldd): Remove useless test on
            architecture and add missing address validity check during LRA.

So fixed?

Assuming it is.

The master branch has been updated by Vladimir Makarov <vmakarov@gcc.gnu.org>:

https://gcc.gnu.org/g:a4670f58ebff805e35268542aac35f9791980954

commit r11-7725-ga4670f58ebff805e35268542aac35f9791980954
Author: Vladimir N. Makarov <vmakarov@redhat.com>
Date:   Thu Mar 18 15:58:26 2021 -0400

    [PR99422] LRA: Use lookup_constraint only for a single constraint in process_address_1.
    
    This is an additional patch for PR99422.  In process_address_1 we
    look only at the first constraint in the 1st alternative
    and ignore all other possibilities.  As we don't know what
    alternative and constraint will be used at this stage, we can be sure
    only for a single constraint with one alternative and should use unknown
    constraint for all other cases.
    
    gcc/ChangeLog:
    
            PR target/99422
            * lra-constraints.c (process_address_1): Use lookup_constraint
            only for a single constraint.