[Bug target/116236] New: [LRA] [M68K] ICE insn does not satisfy its constraints

[Bug target/116236] New: [LRA] [M68K] ICE insn does not satisfy its constraints

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

            Bug ID: 116236
           Summary: [LRA] [M68K] ICE insn does not satisfy its constraints
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Keywords: build, ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: schwab@linux-m68k.org
            Blocks: 113939
  Target Milestone: ---
            Target: m68k-*-*

Created attachment 58839
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58839&action=edit
tzdb.ii

../../../../gcc/xgcc -shared-libgcc -B../../../../gcc -nostdinc++ -std=gnu++20
-fno-implicit-templates -fimplicit-templates -O2 -S tzdb.ii -fPIC -o tzdb.o
../../../../../libstdc++-v3/src/c++20/tzdb.cc: In member function
‘std::chrono::year_month_day
std::chrono::{anonymous}::on_day::pin(std::chrono::year) const’:
../../../../../libstdc++-v3/src/c++20/tzdb.cc:340:7: error: insn does not
satisfy its constraints:
  340 |       }
      |       ^
(insn 261 260 784 22 (set (reg/v:SI 9 %a1 [orig:129 __y0 ] [129])
        (plus:SI (plus:SI (sign_extend:SI (reg/v:HI 9 %a1 [orig:161 y ] [161]))
                (reg:SI 3 %d3 [305]))
            (const_int 1468000 [0x166660]))) "../../include/chrono":1768:18 407
{*lea}
     (nil))
during RTL pass: reload
../../../../../libstdc++-v3/src/c++20/tzdb.cc:340:7: internal compiler error:
in extract_constrain_insn, at recog.cc:2770
0x1ee82ae internal_error(char const*, ...)
        ../../gcc/diagnostic-global-context.cc:491
0x6aafad fancy_abort(char const*, int, char const*)
        ../../gcc/diagnostic.cc:1755
0x697f06 _fatal_insn(char const*, rtx_def const*, char const*, int, char
const*)
        ../../gcc/rtl-error.cc:108
0x697f2f _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
        ../../gcc/rtl-error.cc:118
0x106218d extract_constrain_insn(rtx_insn*)
        ../../gcc/recog.cc:2770
0xeec6c7 check_rtl
        ../../gcc/lra.cc:2189
0xef2142 lra(_IO_FILE*, int)
        ../../gcc/lra.cc:2610
0xe9f727 do_reload
        ../../gcc/ira.cc:5973
0xe9f727 execute
        ../../gcc/ira.cc:6161


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113939
[Bug 113939] Switch m68k to LRA

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #1 from Andreas Schwab <schwab@linux-m68k.org> ---
lea (1468000,%d3,%a1.w),%a1 is not valid, lea (1468000,%a1,%d3.w),%a1 would be.
 Only the index register can be HImode, and the base register must be an
address register.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
m68k makes use of the 'strict_p' parameter to legitimate_address_p which
seems to be only ever set by reload but not LRA.  It might be that both
m68k_legitimate_index_reg_p and m68k_legitimate_base_reg_p need to
treat lra_in_progress the same as strict_p.

Docs say

Legitimate addresses are defined in two variants: a strict variant and a
non-strict one.  The @var{strict} parameter chooses which variant is
desired by the caller.

The strict variant is used in the reload pass.  It must be defined so
that any pseudo-register that has not been allocated a hard register is
considered a memory reference.  This is because in contexts where some
kind of register is required, a pseudo-register with no hard register
must be rejected.  For non-hard registers, the strict variant should look
up the @code{reg_renumber} array; it should then proceed using the hard
register number in the array, or treat the pseudo as a memory reference
if the array holds @code{-1}.

The non-strict variant is used in other passes.  It must be defined to
accept all pseudo-registers in every context where some kind of
register is required.

Maybe LRA should call legitimate_address_p with strict_p == true?

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
struct duration {
  duration(int);
};
struct month {
  operator unsigned();
} _M_m;
struct year {
  short _M_y;
  operator int() { return _M_y; }
} _M_y;
void _M_days_since_epoch() {
  auto __y1(_M_y);
  auto __m1(_M_m);
  auto __j(__m1 < 3);
  auto __y0 = __y1 - __j;
  duration{__y0};
}

> ./cc1plus -quiet tzdb.ii -O -std=gnu++20  -fPIC -I include
tzdb.ii: In function ‘void _M_days_since_epoch()’:
tzdb.ii:17:1: error: insn does not satisfy its constraints:
   17 | }
      | ^
(insn 20 19 21 2 (set (mem:SI (pre_dec:SI (reg/f:SI 15 %sp)) [6  S4 A16])
        (plus:SI (sign_extend:SI (reg:HI 10 %a2 [orig:36 __y1$_M_y ] [36]))
            (reg:SI 0 %d0 [41]))) "tzdb.ii":16:16 77 {pushasi}
     (expr_list:REG_ARGS_SIZE (const_int 4 [0x4])
        (nil)))
during RTL pass: reload
tzdb.ii:17:1: internal compiler error: in extract_constrain_insn, at
recog.cc:2770
0x1ee582e internal_error(char const*, ...)
        /space/rguenther/src/gcc11queue/gcc/diagnostic-global-context.cc:491
0x6abcd5 fancy_abort(char const*, int, char const*)
        /space/rguenther/src/gcc11queue/gcc/diagnostic.cc:1755
0x698c2e _fatal_insn(char const*, rtx_def const*, char const*, int, char
const*)
        /space/rguenther/src/gcc11queue/gcc/rtl-error.cc:108

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
We end up in process_address_1 doing

  /* Target hooks sometimes don't treat extra-constraint addresses as
     legitimate address_operands, so handle them specially.  */
  if (insn_extra_address_constraint (cn)
      && satisfies_address_constraint_p (&ad, cn))
    return change_p;

and that calls legitimate_address_p with strict_p == false and

(plus:SI (sign_extend:SI (reg:HI 36 [ __y1$_M_y ]))
    (reg:SI 41))

returning true and thus we finish process_address_1.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
           Keywords|needs-reduction             |
   Last reconfirmed|                            |2024-08-05

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

Michael Matz <matz at gcc dot gnu.org> changed:

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

--- Comment #5 from Michael Matz <matz at gcc dot gnu.org> ---
re comment #2: yeah.  I don't quite see how LRA can get away (in general)
without
ever calling the target constraint checkers with strict_p == true.

_This_ testcase can be made to work with a mega-hack, by making the generic RTL
analyzers just swap base and index in the cases of ties in the opposite way.
By that IRA selects the right classes from the beginning, LRA heeds those
assignment, and it then so happens that everything turns out right.  Obviously,
as soon as the operands would be swapped then formerly working testcases would 
then start to fail with the hack, so it's not a solution:

diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
index 4158a531bdd..514742d1cae 100644
--- a/gcc/rtlanal.cc
+++ b/gcc/rtlanal.cc
@@ -6727,7 +6727,7 @@ decompose_normal_address (struct address_info *info)
       /* In the event of a tie, assume the base comes first.  */
       if (baseness (*inner_ops[0], info->mode, info->as, PLUS,
                    GET_CODE (*ops[1]))
-         >= baseness (*inner_ops[1], info->mode, info->as, PLUS,
+         > baseness (*inner_ops[1], info->mode, info->as, PLUS,
                       GET_CODE (*ops[0])))
        {
          set_address_base (info, ops[0], inner_ops[0]);

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #6 from Michael Matz <matz at gcc dot gnu.org> ---
This works for me for the full testcase.
a) accept pseudos during lra_in_progress only when they're allocated to the
   right hardreg
b) but _only_ do (a) if the pseudo is not one of those generated _during_ LRA
   (the >= lra_new_regno_start test)
c) when base+disp (and base being allocated the wrong class) needs
   to be reloaded into a new reg, then accept the base as deconstructed,
   don't insist on base and base_term being the same

Case (c) is for insns like the problematic one, where the address needs to be
reloaded for whatever reason.

We have for instance: 
  (insn 49 48 52 6 (set (reg/v:SI 96 [ __y0 ])
        (plus:SI (plus:SI (sign_extend:SI (reg/v:HI 134 [ y ]))
                (reg:SI 144))
            (const_int 1468000 [0x166660]))) "../../include/chrono":1768:18 421
{*lea}

The address operand here is
  (plus:SI (plus:SI (sign_extend:SI (reg/v:HI 134 [ y ]))
                (reg:SI 144))
            (const_int 1468000 [0x166660]))
and decomposition makes ad->base be '(sign_extend:SI (reg/v:HI 134 [ y ]))'
(and hence ad->base_term be '(reg/v:HI 134 [ y ])').  That's suboptimal but
okay.  Further, this base+index+disp operand needs reloading (due to the
suboptimal decomposition and hence suboptimal register class assignment).

So LRA wants to reload base+disp (i.e. (sign_extend:SI (reg/v:HI 134 [ y ]))
plus (const_int 1468000 [0x166660])), via base_plus_disp_to_reg(), but
for whatever reason insists on the base being the same as the base_term,
i.e. be a simple pseudo only.

But in this case it's completely fine (and correct) to just form the
plus of the given base plus offset.  (As I don't know the reason for the
assert it's of course quite possible that this will create problems in other
cases).

I haven't checked more that just compilation of the large and small testcases
here.  Probably stuff is miscompiled.  I lack an overview of how LRA is
supposed
to work in presence of more complicated address forms given that it doesn't
call the backend with any strict checking.

----

diff --git a/gcc/config/m68k/m68k.cc b/gcc/config/m68k/m68k.cc
index 79ba4d5343c..650084566e2 100644
--- a/gcc/config/m68k/m68k.cc
+++ b/gcc/config/m68k/m68k.cc
@@ -306,8 +306,8 @@ static machine_mode m68k_c_mode_for_floating_type (enum tre
 #define TARGET_ASM_OUTPUT_DWARF_DTPREL m68k_output_dwarf_dtprel
 #endif

-#undef TARGET_LRA_P
-#define TARGET_LRA_P hook_bool_void_false
+//#undef TARGET_LRA_P
+//#define TARGET_LRA_P hook_bool_void_false

 #undef TARGET_LEGITIMATE_ADDRESS_P
 #define TARGET_LEGITIMATE_ADDRESS_P    m68k_legitimate_address_p
@@ -2026,20 +2026,27 @@ m68k_output_bftst (rtx zxop0, rtx zxop1, rtx zxop2, rtx
   return code;
 }
 ^L
+extern int lra_new_regno_start;
 /* Return true if X is a legitimate base register.  STRICT_P says
    whether we need strict checking.  */

 bool
 m68k_legitimate_base_reg_p (rtx x, bool strict_p)
 {
+  int regno;
   /* Allow SUBREG everywhere we allow REG.  This results in better code.  */
   if (!strict_p && GET_CODE (x) == SUBREG)
     x = SUBREG_REG (x);

-  return (REG_P (x)
-         && (strict_p
-             ? REGNO_OK_FOR_BASE_P (REGNO (x))
-             : REGNO_OK_FOR_BASE_NONSTRICT_P (REGNO (x))));
+  if (!REG_P (x))
+    return false;
+  regno = REGNO (x);
+  if (lra_in_progress && regno >= lra_new_regno_start)
+    return true;
+
+  return (strict_p || lra_in_progress
+         ? REGNO_OK_FOR_BASE_P (regno)
+         : REGNO_OK_FOR_BASE_NONSTRICT_P (regno));
 }

 /* Return true if X is a legitimate index register.  STRICT_P says
@@ -2048,13 +2055,19 @@ m68k_legitimate_base_reg_p (rtx x, bool strict_p)
 bool
 m68k_legitimate_index_reg_p (rtx x, bool strict_p)
 {
+  int regno;
   if (!strict_p && GET_CODE (x) == SUBREG)
     x = SUBREG_REG (x);

-  return (REG_P (x)
-         && (strict_p
-             ? REGNO_OK_FOR_INDEX_P (REGNO (x))
-             : REGNO_OK_FOR_INDEX_NONSTRICT_P (REGNO (x))));
+  if (!REG_P (x))
+    return false;
+  regno = REGNO (x);
+  if (lra_in_progress && regno >= lra_new_regno_start)
+    return true;
+
+  return (strict_p || lra_in_progress
+         ? REGNO_OK_FOR_INDEX_P (regno)
+         : REGNO_OK_FOR_INDEX_NONSTRICT_P (regno));
 }

 /* Return true if X is a legitimate index expression for a (d8,An,Xn) or
diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
index 92b343fa99a..4ccab7e3395 100644
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -3473,12 +3473,11 @@ base_plus_disp_to_reg (struct address_info *ad, rtx dis
   enum reg_class cl;
   rtx new_reg;

-  lra_assert (ad->base == ad->base_term);
   cl = base_reg_class (ad->mode, ad->as, ad->base_outer_code,
                       get_index_code (ad));
-  new_reg = lra_create_new_reg (GET_MODE (*ad->base_term), NULL_RTX, cl, NULL,
+  new_reg = lra_create_new_reg (GET_MODE (*ad->base), NULL_RTX, cl, NULL,
                                "base + disp");
-  lra_emit_add (new_reg, *ad->base_term, disp);
+  lra_emit_add (new_reg, *ad->base, disp);
   return new_reg;
 }

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

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

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> Docs say
> 
> Legitimate addresses are defined in two variants: a strict variant and a
> non-strict one.  The @var{strict} parameter chooses which variant is
> desired by the caller.
> 
> The strict variant is used in the reload pass.  It must be defined so
> that any pseudo-register that has not been allocated a hard register is
> considered a memory reference.

I don't quite understand this sentence.

Does that mean that legitimate_address_p has to accept MEM as
(part of) a valid address, even when only a hard reg is
allowed as address?

Moreover legitimate_address_p seems outdated / incomplete and
TARGET_ADDR_SPACE_LEGITIMATE_ADDRESS_P the right hook to use.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #8 from Jeffrey A. Law <law at gcc dot gnu.org> ---
If you have a case where a pseudo register is normally valid, but a MEM would
not be valid.  Then you reject that case when strict is on and the pseudo did
not get a hard register.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #9 from Andreas Schwab <schwab@linux-m68k.org> ---
The ADDR_SPACE hooks are only used if the target has support for named address
spaces.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #10 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
What do we do when strict=0 and legitimate_address_p passes a hard register
that is not valid?  Reject it?  Or is that fine and ra will fix it?

(There are cases where passes like insn combine are propagating hard regs into
operands where such registers won't fit, and which will eventually be kicked
out by ra.  Maybe avoiding such propagations gives better code?)

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #11 from Jeffrey A. Law <law at gcc dot gnu.org> ---
In general I would think rejecting is right way to go under the general
guidance of making the predicates and such match what the underlying hardware
can actually do.  So for example, if we we have (mem (reg X)) and we can in
theory propagate d0 resulting in (mem (reg d0)).  Conceptually that might be
worth rejecting.

But you'd have to actually evaluate how it impacts the code -- the m68k port
was written at a time when the standard way to deal with these problems was
punt to reload.  And while a change may make conceptual sense (like tightening
the address checking routines, predicates, etc), such changes may actually make
the resultant code worse.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #12 from Michael Matz <matz at gcc dot gnu.org> ---
But please note that Georg-Johanns question and its ensuing subthread is not
related to this specific m68k problem.  It rather deals with the situation of
what to do in the non-strict variant.

_Here_ we have a case where the target isn't even in the position to reject
an invalid address (or one that definitely _will_ become invalid), because
IRA/LRA don't make use of the strict predicates, and even if it were I don't
see any code dealing with the fall-out of the target saying that
'(plus (zero_extend(a0)) (d0))' would be invalid but 
'(plus (zero_extend(d0)) (a0))' would be valid.
(my hack rectifies this a bit in forcing LRA to just reload the whole
invalid address to another reg, but it doesn't fix all failures of this mode in
the testsuite)

That's why I struggle a bit, I lack the bigger picture, how LRA is envisioned
to deal with these situations given how targets are supposed (and still
documented)
to use the strict/nonstrict distinction of testing for legal operands.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

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

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

--- Comment #13 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #12)
> _Here_ we have a case where the target isn't even in the position to reject
> an invalid address (or one that definitely _will_ become invalid), because
> IRA/LRA don't make use of the strict predicates, and even if it were I don't
> see any code dealing with the fall-out of the target saying that
> '(plus (zero_extend(a0)) (d0))' would be invalid but 
> '(plus (zero_extend(d0)) (a0))' would be valid.
> (my hack rectifies this a bit in forcing LRA to just reload the whole
> invalid address to another reg, but it doesn't fix all failures of this mode
> in the testsuite)
> 
> That's why I struggle a bit, I lack the bigger picture, how LRA is
> envisioned to deal with these situations given how targets are supposed (and
> still documented)
> to use the strict/nonstrict distinction of testing for legal operands.
The generic address decomposition logic should realise that, in:

  (plus (zero_extend R1) R2)

R2 must be the base and R1 must be the index.  Therefore, R2 must be
constrained to BASE_REG_CLASS (or its variants) and R1 must be constrained
to INDEX_REG_CLASS (or its variants).  If that isn't happening then I agree
it's an LRA bug.

(FWIW, I think it's correct that LRA never passes strict_p==true.
The strict_p distinction should go away once reload does.)

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #14 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Richard Sandiford from comment #13)
> (In reply to Michael Matz from comment #12)
> > That's why I struggle a bit, I lack the bigger picture, how LRA is
> > envisioned to deal with these situations given how targets are supposed (and
> > still documented)
> > to use the strict/nonstrict distinction of testing for legal operands.
> The generic address decomposition logic should realise that, in:
> 
>   (plus (zero_extend R1) R2)
> 
> R2 must be the base and R1 must be the index.  Therefore, R2 must be
> constrained to BASE_REG_CLASS (or its variants) and R1 must be constrained
> to INDEX_REG_CLASS (or its variants).

Yes, I considered adding this handling of (zero_extend Rx) to LRA.  I'm
confident
it would fix this instance of the problem, if done correctly (it essentially
already has code to deal with shortening subregs, which is somewhat similar
in structure).

But then I asked myself "what if another target has still
different representations of address?": Should LRA be continuously extended
to deal with these and those representations?  Sure, for optimality that might
be necessary.  But in the grand scheme of things, the target should have the
opportunity, without any changes to LRA code, to be made completely working
with whatever representations it's using internally.  It needs to have a way
to say "this address, if written with the assigned hardregs, will be invalid,
do something".  Alternatively (and better) it needs to have a way to say "this
address, while structurally valid, will need these regsets in those reg
operands", generally (i.e. it should be possible to have targets with e.g. 4
register operands, or such).

If that's not possible then the design of the LRA-target interface is not yet
complete IMHO.

> If that isn't happening then I agree it's an LRA bug.

It's not happening simply because LRA doesn't handle {ZERO,SIGN}_EXTEND
specially at all, it's opaque.  But the question is: should it have to?  I'm
not
sure I agree with you, because in my mind it points to a deficiency in the
target interface.

> (FWIW, I think it's correct that LRA never passes strict_p==true.
> The strict_p distinction should go away once reload does.)

But how could it?  The various target hooks are called from LRA when the
pseudos are _not_ yet replaced with their MEMs or their hardregs.  How would
the target know to do this substitution internally for correctness checking?
How would it know that one pseudo is equivalent to a MEM, and another to a
hardreg?  Looking at lra_in_progress as some targets are currently doing (and
my hack does)?  That doesn't sound like a good design, it's simply a hidden
parameter for the target hook in a global variable.

So, how else?  Substituting before the hooks are asked is just busy work,
not asking the target at all means continuously adding stuff to LRA whenever a
target wants something more.

Hmm.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #15 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #14)
> (In reply to Richard Sandiford from comment #13)
> > (In reply to Michael Matz from comment #12)
> > > That's why I struggle a bit, I lack the bigger picture, how LRA is
> > > envisioned to deal with these situations given how targets are supposed (and
> > > still documented)
> > > to use the strict/nonstrict distinction of testing for legal operands.
> > The generic address decomposition logic should realise that, in:
> > 
> >   (plus (zero_extend R1) R2)
> > 
> > R2 must be the base and R1 must be the index.  Therefore, R2 must be
> > constrained to BASE_REG_CLASS (or its variants) and R1 must be constrained
> > to INDEX_REG_CLASS (or its variants).
> 
> Yes, I considered adding this handling of (zero_extend Rx) to LRA.  I'm
> confident
> it would fix this instance of the problem, if done correctly (it essentially
> already has code to deal with shortening subregs, which is somewhat similar
> in structure).
It should already be present, via the decompose_*_address mechanism.

FWIW, aarch64 also supports (mem (plus (zero_extend R1) R2)) and works with
LRA, so I don't think there's a fundamental limitation.

> But then I asked myself "what if another target has still
> different representations of address?": Should LRA be continuously extended
> to deal with these and those representations?  Sure, for optimality that
> might
> be necessary.  But in the grand scheme of things, the target should have the
> opportunity, without any changes to LRA code, to be made completely working
> with whatever representations it's using internally.  It needs to have a way
> to say "this address, if written with the assigned hardregs, will be
> invalid, do something".  Alternatively (and better) it needs to have a way
> to say "this
> address, while structurally valid, will need these regsets in those reg
> operands", generally (i.e. it should be possible to have targets with e.g. 4
> register operands, or such).
> 
> If that's not possible then the design of the LRA-target interface is not
> yet complete IMHO.
Yeah, the current approach is the latter one (which I agree is better). 
 legitimate_address_p answers the question “is this address structurally
valid?” while BASE_REG_CLASS and INDEX_REG_CLASS specify the regsets that
should be used to reload registers in structurally valid addresses.

Like you say, in practice it has to be done by using regsets, since the RA
needs to know “what do I need to do to make this valid?”.  It shouldn't have to
use trial and error (trying particular hard registers to see if they're valid).

> > (FWIW, I think it's correct that LRA never passes strict_p==true.
> > The strict_p distinction should go away once reload does.)
> 
> But how could it?  The various target hooks are called from LRA when the
> pseudos are _not_ yet replaced with their MEMs or their hardregs.  How would
> the target know to do this substitution internally for correctness checking?
> How would it know that one pseudo is equivalent to a MEM, and another to a
> hardreg?  Looking at lra_in_progress as some targets are currently doing (and
> my hack does)?  That doesn't sound like a good design, it's simply a hidden
> parameter for the target hook in a global variable.
> 
> So, how else?  Substituting before the hooks are asked is just busy work,
> not asking the target at all means continuously adding stuff to LRA whenever
> a
> target wants something more.
BASE_REG_CLASS and INDEX_REG_CLASS (and their variants) specify what needs to
happen for an address that doesn't currently use hard registers.  And
legitimate_address_p should test whether addresses that *do* currently use hard
registers are using the right registers.

There should be no need for the hooks to do their own pseudo-to-hard-register
lookup.  The idea that one pseudo register maps to one hard register is very
simplistic and doesn't take region-based allocation or inheritance into
account.  IMO it's a concept that should go away when reload does.

And it doesn't make sense IMO to accept (say) %aN index registers or %dN base
registers at any stage, even before allocation.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #16 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Richard Sandiford from comment #15)
> > Yes, I considered adding this handling of (zero_extend Rx) to LRA.  I'm
> > confident
> > it would fix this instance of the problem, if done correctly (it essentially
> > already has code to deal with shortening subregs, which is somewhat similar
> > in structure).
> It should already be present, via the decompose_*_address mechanism.

As get_index_term doesn't accept any _EXTEND as index outer code I don't see
how (currently).  It basically gets into the tie-breaker of leaving the
operands
after the loop in decompose_normal_address for baseness() to decide which of
course gets it "wrong" on m68k (see my comment #5 where I checked :) ) ...

> FWIW, aarch64 also supports (mem (plus (zero_extend R1) R2)) and works with
> LRA, so I don't think there's a fundamental limitation.

... but obviously gets it right by luck on aarch64.

> > invalid, do something".  Alternatively (and better) it needs to have a way
> > to say "this
> > address, while structurally valid, will need these regsets in those reg
> > operands", generally (i.e. it should be possible to have targets with e.g. 4
> > register operands, or such).
> > 
> > If that's not possible then the design of the LRA-target interface is not
> > yet complete IMHO.
> Yeah, the current approach is the latter one (which I agree is better). 
>  legitimate_address_p answers the question “is this address structurally
> valid?” while BASE_REG_CLASS and INDEX_REG_CLASS specify the regsets that
> should be used to reload registers in structurally valid addresses.

Well, that then rules out targets that allow three registers in addresses.
Or ones that have more complicated validity rules than "op1 here, op2 there".
E.g. "when op1 is an even-numbered register, then op2 needs to be odd-numbered,
and vice versa".  Contrived, sure, but ISA might want to save a bit here or
there in encoding.

> Like you say, in practice it has to be done by using regsets, since the RA
> needs to know “what do I need to do to make this valid?”.  It shouldn't have
> to use trial and error (trying particular hard registers to see if they're
> valid).

Agreed.  I just think that BASE/INDEX_REG_CLASS as the only way of
communicating that from the target to LRA is quite constraining.  It means
(like
here) to always having to adjust LRA whenever a new target with other forms
comes along.  Basically: when the target has to look at the structure of
an address to check validity (reasonably so), then it irks me that also
LRA needs to get at the inner structure of those addresses for correctness.
The latter part, the correctness, is what triggers me.  If it's necessary for
optimality: sure.  But if LRA needs to be extended for correctness, then, ...
meh.

> > [how should stuff work without strict_p?]
> 
> BASE_REG_CLASS and INDEX_REG_CLASS (and their variants) specify what needs
> to happen for an address that doesn't currently use hard registers.  And
> legitimate_address_p should test whether addresses that *do* currently use
> hard registers are using the right registers.

(Hmm, that's quite a deviation from past, that eventually should be documented
:) )

> There should be no need for the hooks to do their own
> pseudo-to-hard-register lookup.  The idea that one pseudo register maps to
> one hard register is very simplistic and doesn't take region-based
> allocation or inheritance into account.  IMO it's a concept that should go
> away when reload does.

Well, okay, I can live with that general direction and idea.  But then I think
the current interface (basically just BASE/INDEX_REG_CLASS, with LRA
"magically"
having to do the right decision of what is index and what is base, and nothing
at all possible for more than two-reg-addresses) is a bit too simplistic.

It should then rather be more like an interface where the target gets an
address, checks for structural validity, and if valid abstractly, then calls
back into (to be provided) means in the reg-allocator to properly set regsets
for each pseudo it finds in the address.  Instead of the target having to rely
on generic code in the reg-allocator to hopefully "correctly" deconstruct the
address in just the right way to infer the proper reg-classes by itself.

> And it doesn't make sense IMO to accept (say) %aN index registers or %dN
> base registers at any stage, even before allocation.

You mean if they are hardregs already?  No, of course not, but I never was
talking about addresses already having hardregs.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rsandifo at gcc dot gnu.org

--- Comment #17 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #16)
> (In reply to Richard Sandiford from comment #15)
> > > Yes, I considered adding this handling of (zero_extend Rx) to LRA.  I'm
> > > confident
> > > it would fix this instance of the problem, if done correctly (it essentially
> > > already has code to deal with shortening subregs, which is somewhat similar
> > > in structure).
> > It should already be present, via the decompose_*_address mechanism.
> 
> As get_index_term doesn't accept any _EXTEND as index outer code I don't see
> how (currently).  It basically gets into the tie-breaker of leaving the
> operands
> after the loop in decompose_normal_address for baseness() to decide which of
> course gets it "wrong" on m68k (see my comment #5 where I checked :) ) ...
> 
> > FWIW, aarch64 also supports (mem (plus (zero_extend R1) R2)) and works with
> > LRA, so I don't think there's a fundamental limitation.
> 
> ... but obviously gets it right by luck on aarch64.
Hmm, ok.  I think that's probably my bug then, so... taking.

> > > invalid, do something".  Alternatively (and better) it needs to have a way
> > > to say "this
> > > address, while structurally valid, will need these regsets in those reg
> > > operands", generally (i.e. it should be possible to have targets with e.g. 4
> > > register operands, or such).
> > > 
> > > If that's not possible then the design of the LRA-target interface is not
> > > yet complete IMHO.
> > Yeah, the current approach is the latter one (which I agree is better). 
> >  legitimate_address_p answers the question “is this address structurally
> > valid?” while BASE_REG_CLASS and INDEX_REG_CLASS specify the regsets that
> > should be used to reload registers in structurally valid addresses.
> 
> Well, that then rules out targets that allow three registers in addresses.
> Or ones that have more complicated validity rules than "op1 here, op2 there".
> E.g. "when op1 is an even-numbered register, then op2 needs to be
> odd-numbered,
> and vice versa".  Contrived, sure, but ISA might want to save a bit here or
> there in encoding.
> 
> > Like you say, in practice it has to be done by using regsets, since the RA
> > needs to know “what do I need to do to make this valid?”.  It shouldn't have
> > to use trial and error (trying particular hard registers to see if they're
> > valid).
> 
> Agreed.  I just think that BASE/INDEX_REG_CLASS as the only way of
> communicating that from the target to LRA is quite constraining.  It means
> (like
> here) to always having to adjust LRA whenever a new target with other forms
> comes along.  Basically: when the target has to look at the structure of
> an address to check validity (reasonably so), then it irks me that also
> LRA needs to get at the inner structure of those addresses for correctness.
> The latter part, the correctness, is what triggers me.  If it's necessary for
> optimality: sure.  But if LRA needs to be extended for correctness, then, ...
> meh.
But this is how it's always worked.  The corresponding reload code is in
find_reloads_address_1, which similarly tries to tear apart an address, work
out whether something is a base or an index, and use that to calculate the
appropriate register class.  ISTM this PR is just about an inconsistency
between the base/index identification in LRA vs reload.

The reg_renumber indirection performed by GO_IF_LEGITIMATE_ADDRESS for reload
only handled the initial register allocation.  It didn't help for pseudos that
were spilled, or were allocated to the wrong class.  The
BASE_REG_CLASS/INDEX_REG_CLASS mechanism was always required to work for
correctness, not just optimisation.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #18 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
Created attachment 58927
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58927&action=edit
Candidate patch

Could you try the attached patch?  It seems to fix the reduced testcase for me.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #19 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
Of course, immediately after posting I realise it should be address_mode
instead of pointer_mode, but that shouldn't affect m68k.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #20 from Andreas Schwab <schwab@linux-m68k.org> ---
With that patch I can successfully build the C++ target libraries with LRA
enabled.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[matz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #21 from Michael Matz <matz at gcc dot gnu.org> ---
(In reply to Richard Sandiford from comment #17)
> > But if LRA needs to be extended for correctness, then, ... meh.
> But this is how it's always worked.  The corresponding reload code is in
> find_reloads_address_1, which similarly tries to tear apart an address, work
> out whether something is a base or an index, and use that to calculate the
> appropriate register class.  ISTM this PR is just about an inconsistency
> between the base/index identification in LRA vs reload.
> 
> The reg_renumber indirection performed by GO_IF_LEGITIMATE_ADDRESS for
> reload only handled the initial register allocation.  It didn't help for
> pseudos that were spilled, or were allocated to the wrong class.  The
> BASE_REG_CLASS/INDEX_REG_CLASS mechanism was always required to work for
> correctness, not just optimisation.

Yes, and I can't say that I ever liked it very much in reload either (for
basically the same reasons I'm whining now here) :-)  Perhaps I have just hoped
LRA would come up with some other solution.  Anyway, yes handling the
decomposition in just the right way in LRA/rtlanal fixes the problem, so case
closed.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #22 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
(In reply to Michael Matz from comment #21)
> (In reply to Richard Sandiford from comment #17)
> > > But if LRA needs to be extended for correctness, then, ... meh.
> > But this is how it's always worked.  The corresponding reload code is in
> > find_reloads_address_1, which similarly tries to tear apart an address, work
> > out whether something is a base or an index, and use that to calculate the
> > appropriate register class.  ISTM this PR is just about an inconsistency
> > between the base/index identification in LRA vs reload.
> > 
> > The reg_renumber indirection performed by GO_IF_LEGITIMATE_ADDRESS for
> > reload only handled the initial register allocation.  It didn't help for
> > pseudos that were spilled, or were allocated to the wrong class.  The
> > BASE_REG_CLASS/INDEX_REG_CLASS mechanism was always required to work for
> > correctness, not just optimisation.
> 
> Yes, and I can't say that I ever liked it very much in reload either (for
> basically the same reasons I'm whining now here) :-)  Perhaps I have just
> hoped
> LRA would come up with some other solution.
Yeah, I definitely agree that a different solution would nicer.  Part of the
problem with the current approach is obvious from the way we had
BASE_REG_CLASS, then (when that wasn't enough) MODE_BASE_REG_CLASS, then
MODE_BASE_REG_REG_CLASS, and finally(?) MODE_CODE_BASE_REG_CLASS. :)

So I agree that it would be better for the target to say what parts of an
address need reloading, and how, without any of the current guesswork.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #23 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:3673b7054ec268c445620b9c52d25e65bc9a7f96

commit r15-2937-g3673b7054ec268c445620b9c52d25e65bc9a7f96
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Thu Aug 15 16:54:03 2024 +0100

    Tweak base/index disambiguation in decompose_normal_address [PR116236]

    The PR points out that, for an address like:

      (plus (zero_extend X) Y)

    decompose_normal_address doesn't establish a strong preference
    between treating X as the base or Y as the base.  As the comment
    in the patch says, zero_extend isn't enough on its own to assume
    an index, at least not on POINTERS_EXTEND_UNSIGNED targets.
    But in a construct like the one above, X and Y have different modes,
    and it seems reasonable to assume that the one with the expected
    address mode is the base.

    This matters on targets like m68k that support index extension
    and that require different classes for bases and indices.

    gcc/
            PR middle-end/116236
            * rtlanal.cc (decompose_normal_address): Try to distinguish
            bases and indices based on mode, before resorting to "baseness".

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

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

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

--- Comment #24 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
Hopefully fixed, but please let me know if this code causes similar problems
elsewhere.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #25 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Sandiford from comment #24)
> Hopefully fixed, but please let me know if this code causes similar problems
> elsewhere.

I think there was a corresponding bug on the AVR side, not sure if that's now
also resolved.

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #26 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #25)
> I think there was a corresponding bug on the AVR side, not sure if that's
> now also resolved.
As far as I understand, the AVR issue is of a different kind

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116321#c4

[Bug target/116236] [LRA] [M68K] ICE insn does not satisfy its constraints

[hp at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116236

--- Comment #27 from Hans-Peter Nilsson <hp at gcc dot gnu.org> ---
(In reply to GCC Commits from comment #23)
> The trunk branch has been updated by Richard Sandiford
> <rsandifo@gcc.gnu.org>:
> 
> https://gcc.gnu.org/g:3673b7054ec268c445620b9c52d25e65bc9a7f96
> 
> commit r15-2937-g3673b7054ec268c445620b9c52d25e65bc9a7f96

I had a hunch, verified by reverting this commit locally:
This commit also fixed the underlying LRA bug in PR115883.  Nice!