Re: Illegitimate address in REG_EQUAL note: legal?

Re: Illegitimate address in REG_EQUAL note: legal?

[Ulrich Weigand]

Jan Hubicka wrote:

>> gcse constant-propagation can substitute a constant
>> term for a register into a memory address, with the
>> resulting address being illegitimate.
>
>I guess it is valid.  We don't require REG_EQUIV expressions to match the
>target constraints, we want them to describe directly what is going on.

OK, I see.

>> Unfortunately, this note gets promoted to a REG_EQUIV
>> note later on, and when reload sees this note, it will
>> create a reg_equiv_address entry for the pseudo.
>
>I guess this is the problem - the REG_EQUIV notes have different
>meaning for pre-reload and for reload/postreload, so I guess
>somewhere here we should kill REG_EQUIV notes with values invalid
>for reload.
>
>SImilarry I would guess we should kill REG_EQUALs somewhere on
>the progress as well, as they are invalid and some passes
>common for post/pre reload, like cfg_cleanup may get confused.

It looks like reload assumes REG_EQUIV notes to be either a
MEM (which is then implicitly assumed to be a legitimate
memory operand), or else a constant or stack pointer offset
(as accepted by function_invariant_p).

So, I think that local-alloc should only convert such REG_EQUAL
notes to REG_EQUIV that fit these expectations.  The following
patch implements this check (and fixes my test case).

Index: local-alloc.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/local-alloc.c,v
retrieving revision 1.102
diff -c -p -r1.102 local-alloc.c
*** local-alloc.c   2002/02/19 02:53:13  1.102
--- local-alloc.c   2002/03/05 17:37:49
*************** update_equiv_regs ()
*** 946,951 ****
--- 946,959 ----
       if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
         note = NULL_RTX;

+      /* Reload assumes that REG_EQUIV notes contain either a
+         legitimate memory operand, or else a constant rtx as
+         accepted by function_invariant_p.  Take care to not
+         generate anything else.  */
+      if (note && !memory_operand (XEXP (note, 0), VOIDmode)
+          && !function_invariant_p (XEXP (note, 0)))
+        note = NULL_RTX;
+
       if (REG_N_SETS (regno) != 1
           && (! note
            || rtx_varies_p (XEXP (note, 0), 0)

Do you think this could be a step in the right direction?  I don't
completely understand all the implicit assumptions made by reload
and others, so I'm somewhat hesitant here ...


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

Re: Illegitimate address in REG_EQUAL note: legal?

[Jan Hubicka]

> Index: local-alloc.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/local-alloc.c,v
> retrieving revision 1.102
> diff -c -p -r1.102 local-alloc.c
> *** local-alloc.c   2002/02/19 02:53:13  1.102
> --- local-alloc.c   2002/03/05 17:37:49
> *************** update_equiv_regs ()
> *** 946,951 ****
> --- 946,959 ----
>        if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>          note = NULL_RTX;
> 
> +      /* Reload assumes that REG_EQUIV notes contain either a
> +         legitimate memory operand, or else a constant rtx as
> +         accepted by function_invariant_p.  Take care to not
> +         generate anything else.  */
> +      if (note && !memory_operand (XEXP (note, 0), VOIDmode)
> +          && !function_invariant_p (XEXP (note, 0)))
> +        note = NULL_RTX;
> +
>        if (REG_N_SETS (regno) != 1
>            && (! note
>             || rtx_varies_p (XEXP (note, 0), 0)
> 
> Do you think this could be a step in the right direction?  I don't
> completely understand all the implicit assumptions made by reload
> and others, so I'm somewhat hesitant here ...

Neighter I do, but I would guess this is fine.
We also create REG_EQUIV notes pre-reload for stack slots. There
is some risc that we will manage gcse to mangle these REG_EQUIVs
to not match reload's point of view, but I can't come with any sane
testcase so I guess it won't happen.

Honza
> 
> 
> Mit freundlichen Gruessen / Best Regards
> 
> Ulrich Weigand
> 
> --
>   Dr. Ulrich Weigand
>   Linux for S/390 Design & Development
>   IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
>   Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

Re: Illegitimate address in REG_EQUAL note: legal?

[Jan Hubicka]

> Hello,
> 
> another problem with the current 3.1 on s390: 
> 
> gcse constant-propagation can substitute a constant
> term for a register into a memory address, with the
> resulting address being illegitimate.  

I guess it is valid.  We don't require REG_EQUIV expressions to match the
target constraints, we want them to describe directly what is going on.
> 
> (Specifically, gcse has pulled a (const (unspec ...))
> construct used to specify a GOT offset larger than 4K
> out of the constant pool and placed it inline into
> the address.  This does not work.)
> 
> Now, try_replace_reg does notice that something 
> isn't OK, as validate_replace_src fails.  However,
> if that happens, the illegitimate address is still
> placed into a REG_EQUAL note which is attached to
> the insn.
> 
> Unfortunately, this note gets promoted to a REG_EQUIV
> note later on, and when reload sees this note, it will
> create a reg_equiv_address entry for the pseudo.

I guess this is the problem - the REG_EQUIV notes have different
meaning for pre-reload and for reload/postreload, so I guess
somewhere here we should kill REG_EQUIV notes with values invalid
for reload.
SImilarry I would guess we should kill REG_EQUALs somewhere on
the progress as well, as they are invalid and some passes
common for post/pre reload, like cfg_cleanup may get confused.

Honza

> However, while req_equiv_address addresses are presumed
> to be illegitimate, reload does assume they are so for
> a couple of special reasons (i.e. not strictly legitimate,
> CONST_INT offset out of range etc.)
> 
> In this particular case, the address doesn't fit into
> any of these categories, and so find_reloads_address
> doesn't handle it correctly.  This results in the
> address showing up un-reloaded in the rtx after reload,
> causing an ICE in cleanup_subreg_operands due to an
> unrecognized insn.
> 
> 
> I'm at a loss to decide which of the involved parties
> is at fault here.  Either:
> 
> - REG_EQUAL notes should never contain illegitimate
>   memory addresses.
> 
>   In this case gcse try_replace_reg needs to be fixed.
>   [ Why does it keep things it *knows* to be invalid
>   around anyway?  The comment says it's done so as
>   'to not lose information'.  Am I missing something
>   here? ]
> 
> - REG_EQUAL notes may contain illegitimate addresses,
>   but REG_EQUIV notes not.
> 
>   In this case local-alloc probably needs to add
>   checks to ensure it only promotes valid REG_EQUAL
>   notes to REG_EQUIV.
> 
> - Both REG_EQUIV and REG_EQUAL notes may contain
>   illegitimate addresses.
> 
>   Then reload need to either handle such addresses
>   correctly in find_reloads_address, or else check
>   the note in advance and ignore it if is isn't
>   legitimate.
> 
> Any suggestions?
> 
> Bye,
> Ulrich
> 
> -- 
>   Dr. Ulrich Weigand
>   weigand@informatik.uni-erlangen.de

Illegitimate address in REG_EQUAL note: legal?

[Ulrich Weigand]

Hello,

another problem with the current 3.1 on s390: 

gcse constant-propagation can substitute a constant
term for a register into a memory address, with the
resulting address being illegitimate.  

(Specifically, gcse has pulled a (const (unspec ...))
construct used to specify a GOT offset larger than 4K
out of the constant pool and placed it inline into
the address.  This does not work.)

Now, try_replace_reg does notice that something 
isn't OK, as validate_replace_src fails.  However,
if that happens, the illegitimate address is still
placed into a REG_EQUAL note which is attached to
the insn.

Unfortunately, this note gets promoted to a REG_EQUIV
note later on, and when reload sees this note, it will
create a reg_equiv_address entry for the pseudo.
However, while req_equiv_address addresses are presumed
to be illegitimate, reload does assume they are so for
a couple of special reasons (i.e. not strictly legitimate,
CONST_INT offset out of range etc.)

In this particular case, the address doesn't fit into
any of these categories, and so find_reloads_address
doesn't handle it correctly.  This results in the
address showing up un-reloaded in the rtx after reload,
causing an ICE in cleanup_subreg_operands due to an
unrecognized insn.


I'm at a loss to decide which of the involved parties
is at fault here.  Either:

- REG_EQUAL notes should never contain illegitimate
  memory addresses.

  In this case gcse try_replace_reg needs to be fixed.
  [ Why does it keep things it *knows* to be invalid
  around anyway?  The comment says it's done so as
  'to not lose information'.  Am I missing something
  here? ]

- REG_EQUAL notes may contain illegitimate addresses,
  but REG_EQUIV notes not.

  In this case local-alloc probably needs to add
  checks to ensure it only promotes valid REG_EQUAL
  notes to REG_EQUIV.

- Both REG_EQUIV and REG_EQUAL notes may contain
  illegitimate addresses.

  Then reload need to either handle such addresses
  correctly in find_reloads_address, or else check
  the note in advance and ignore it if is isn't
  legitimate.

Any suggestions?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de