Re: [PATCH] i386: Fix up *add_1 [PR125469]

[Uros Bizjak]

On Thu, May 28, 2026 at 11:31 AM Uros Bizjak <ubiz...@gmail.com> wrote:
>
> On Thu, May 28, 2026 at 10:14 AM Uros Bizjak <ubiz...@gmail.com> wrote:
> >
> > On Thu, May 28, 2026 at 9:45 AM Jakub Jelinek <ja...@redhat.com> wrote:
> > >
> > > Hi!
> > >
> > > The following testcase ICEs, because combine matches
> > > (set (reg:DI 108) (plus:DI (reg:DI 104 [ s ]) (subreg:DI (reg:TI 103 [ _2 
> > > ]) 8)))
> > > Now, because ix86_validate_address_register has:
> > > 12038         /* Don't allow SUBREGs that span more than a word.  It can
> > > 12039            lead to spill failures when the register is one word out
> > > 12040            of a two word structure.  */
> > > 12041         if (GET_MODE_SIZE (mode) > UNITS_PER_WORD)
> > > 12042           return NULL_RTX;
> > > this isn't recognized as *leadi, but is recognized as *adddi_1_nf pattern
> > > instead.  Now, later on the RA turns it into:
> > > (set (reg:DI 2 cx [108]) (plus:DI (reg:DI 0 ax [orig:104 s ] [104]) 
> > > (reg:DI 5 di [ _2+8 ])))
> > > which would be valid *leadi, but given that INSN_CODE is already set to 
> > > the
> > > *adddi_1_nf and that also satisfies it, nothing re-recognizes it as 
> > > *leadi.
> > > But in that case without TARGET_APX_NDD the pattern has return "#";
> > > That is a bug, because there is no splitter to split that
> > > (set (reg:DI 2 cx [108]) (plus:DI (reg:DI 0 ax [orig:104 s ] [104]) 
> > > (reg:DI 5 di [ _2+8 ])))
> > > into itself so that it is re-recognized as *leadi, so it just ICEs.
> > > I think having a splitter to split to the same thing would be just weird, 
> > > so
> > > this just outputs lea insn directly.
> > >
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/16/15?
> > >
> > > 2026-05-28  Jakub Jelinek  <ja...@redhat.com>
> > >
> > >         PR target/125469
> > >         * config/i386/i386.md (*add<mode>_1<nf_name>): Don't return "#" 
> > > for
> > >         the lea non-TARGET_APX_NDD case, instead emit a lea directly.
> > >
> > >         * gcc.target/i386/apx-nf-pr125469.c: New test.
> >
> > OK. I also wonder if the check in ix86_validate_address_register can
> > be removed, there were many RA improvements in the last ... decade
> > (?).
>
> Hm, OTOH, is this condition correct:
>
>     case TYPE_LEA:
>       if (TARGET_APX_NDD && <nf_applied>)
>     return "%{nf%} add{<imodesuffix>}\t{%2, %1, %0|%0, %1, %2}";
>       else
>     return "#";
>
> Should it be "TARGET_APX_NDD || <nf_applied>"?

No, existing condition is OK.

Uros.