simplify_subreg issues

simplify_subreg issues

[Mark Mitchell]

A patch of mine is triggering failures in builtin-complex-1.c on x86.
The test code is:

  extern float _Complex conjf (float _Complex);

  int
  main (void)
  {
    volatile float _Complex fc = 1.0F + 2.0iF;
    if (conjf (fc) != 1.0F - 2.0iF)
      abort ();
  }

Before my patch, we did not mark the `MEM' for `fc' as MEM_VOLATILE_P.
My patch corrected this bug.

However, we now generate:

  (insn 13 11 0 (set (subreg:SF (mem/v/f:SC (plus:SI (reg/f:SI 54 virtual-stack-vars)
		      (const_int -8 [0xfffffff8])) 0) 4)
	  (mem/u/f:SF (const:SI (plus:SI (symbol_ref:SI ("*.LC0"))
		      (const_int 4 [0x4]))) 0)) -1 (nil)
      (nil))

Later we abort in disgust on the SUBREG of a MEM.
 
To reproduce this bug, look at the mainline -- not the branch.  Set a
breakpoint here, in emit_move_insn_1:

      /* Don't split destination if it is a stack push.  */
      int stack = push_operand (x, GET_MODE (x));

Look at how `imagpart_x' is computed a while down.  The call to
gen_imagpart calls gen_highpart calls simplify_gen_subreg calls
simplify_subreg.

This code in simplify_subreg:

  if (GET_CODE (op) == MEM
      && ! mode_dependent_address_p (XEXP (op, 0))
      && ! MEM_VOLATILE_P (op)
      && GET_MODE_SIZE (outermode) <= GET_MODE_SIZE (GET_MODE (op)))
    {
      rtx new;

      new = gen_rtx_MEM (outermode, plus_constant (XEXP (op, 0), byte));
      MEM_COPY_ATTRIBUTES (new, op);
      return new;
    }

no longer fires, due to the volatility.  So, we make a real SUBREG.

I don't know how to fix this.  We can revert my patch, but that will
reintroduce the bugs it was trying to fix, so I would be happier if we
could figure out to fix the real problem.

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: simplify_subreg issues

[Geoff Keating]

> Cc: gcc@gcc.gnu.org
> X-URL: http://www.codesourcery.com
> Organization: CodeSourcery, LLC
> From: Mark Mitchell <mark@codesourcery.com>
> Date: Mon, 11 Jun 2001 19:09:07 -0700
> X-Dispatcher: imput version 990425(IM115)
> 
> 
> A patch of mine is triggering failures in builtin-complex-1.c on x86.
> The test code is:
> 
>   extern float _Complex conjf (float _Complex);
> 
>   int
>   main (void)
>   {
>     volatile float _Complex fc = 1.0F + 2.0iF;
>     if (conjf (fc) != 1.0F - 2.0iF)
>       abort ();
>   }
> 
> Before my patch, we did not mark the `MEM' for `fc' as MEM_VOLATILE_P.
> My patch corrected this bug.
> 
> However, we now generate:
> 
>   (insn 13 11 0 (set (subreg:SF (mem/v/f:SC (plus:SI (reg/f:SI 54 virtual-stack-vars)
> 		      (const_int -8 [0xfffffff8])) 0) 4)
> 	  (mem/u/f:SF (const:SI (plus:SI (symbol_ref:SI ("*.LC0"))
> 		      (const_int 4 [0x4]))) 0)) -1 (nil)
>       (nil))
> 
> Later we abort in disgust on the SUBREG of a MEM.
>  
> To reproduce this bug, look at the mainline -- not the branch.  Set a
> breakpoint here, in emit_move_insn_1:
> 
>       /* Don't split destination if it is a stack push.  */
>       int stack = push_operand (x, GET_MODE (x));
> 
> Look at how `imagpart_x' is computed a while down.  The call to
> gen_imagpart calls gen_highpart calls simplify_gen_subreg calls
> simplify_subreg.
> 
> This code in simplify_subreg:
> 
>   if (GET_CODE (op) == MEM
>       && ! mode_dependent_address_p (XEXP (op, 0))
>       && ! MEM_VOLATILE_P (op)
>       && GET_MODE_SIZE (outermode) <= GET_MODE_SIZE (GET_MODE (op)))
>     {
>       rtx new;
> 
>       new = gen_rtx_MEM (outermode, plus_constant (XEXP (op, 0), byte));
>       MEM_COPY_ATTRIBUTES (new, op);
>       return new;
>     }
> 
> no longer fires, due to the volatility.  So, we make a real SUBREG.
> 
> I don't know how to fix this.  We can revert my patch, but that will
> reintroduce the bugs it was trying to fix, so I would be happier if we
> could figure out to fix the real problem.

The underlying problem is that 'volatile' doesn't make sense in
conjunction with 'complex'.  We can't do complex loads or stores.  So,
actually in this case the optimisation would be correct, because what
we want to have is

(set (mem/v:SF ...) ...)

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: simplify_subreg issues

[Mark Mitchell]

> The underlying problem is that 'volatile' doesn't make sense in
> conjunction with 'complex'.  We can't do complex loads or stores.  So,
> actually in this case the optimisation would be correct, because what
> we want to have is
>
> (set (mem/v:SF ...) ...)
>

I don't really understand what we do with `volatile' on a structure
type, in general, or what the standard says we have to do.

I guess one would like to avoid `f(y)' -- where `y' is some
volatile complex type -- generating more than one reference to any
word in `y'.

Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
test should be done only in conjuction with some additional check
that says `and is not a complex type'?

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: simplify_subreg issues

[Geoff Keating]

> Date: Mon, 11 Jun 2001 21:39:36 -0700
> From: Mark Mitchell <mark@codesourcery.com>
> cc: "kenner@vlsi1.ultra.nyu.edu" <kenner@vlsi1.ultra.nyu.edu>,
>         "jh@suse.cz" <jh@suse.cz>, "gcc@gcc.gnu.org" <gcc@gcc.gnu.org>
> Content-Disposition: inline
> 
> 
> > The underlying problem is that 'volatile' doesn't make sense in
> > conjunction with 'complex'.  We can't do complex loads or stores.  So,
> > actually in this case the optimisation would be correct, because what
> > we want to have is
> >
> > (set (mem/v:SF ...) ...)
> >
> 
> I don't really understand what we do with `volatile' on a structure
> type, in general, or what the standard says we have to do.

The standard says nothing meaningful about 'volatile'.  We can do
whatever we think would be useful or convenient.

> I guess one would like to avoid `f(y)' -- where `y' is some
> volatile complex type -- generating more than one reference to any
> word in `y'.

That sounds reasonable.

> Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
> or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
> test should be done only in conjuction with some additional check
> that says `and is not a complex type'?

The last.  Something like HAVE_mov* where '*' is the mode, I think.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: simplify_subreg issues

[Mark Mitchell]

>> Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
>> or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
>> test should be done only in conjuction with some additional check
>> that says `and is not a complex type'?
>
> The last.  Something like HAVE_mov* where '*' is the mode, I think.

OK.  I assume that we can never get to this code with a user-defined
type, even one like `struct S { float f; float g; };', followed by
a structure copy of an instance of S?  If we can it seems like we should 
handle that similarly, but I think we're not very clever, and just
make S have BLKmode.

I guess this still feels a little fragile to me -- we're calling
a function with simplify_gen_subreg in a situation where we definitely
do not want `subreg' -- the manual says we should have no subregs
with a MEM in them until much later in the game.  (We're doing this
during the initial tree->rtl conversion.)  In fact, I wonder if
simplify_gen_subreg shouldn't abort in the case that it is given
a MEM that it cannot simplify away.  Oh, well.

What about just checking to see if the mode is one of the complex
modes, and ignoring MEM_VOLATILE_P in that case?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: simplify_subreg issues

[Jan Hubicka]

> I don't know how to fix this.  We can revert my patch, but that will
> reintroduce the bugs it was trying to fix, so I would be happier if we
> could figure out to fix the real problem.
Sure, I will try to proceed on fixing the real problem.
The volatility check in simplify_subreg looks somewhat dubious for me.
When I was unifying the code, some copies did contain it, some didn't.
I decided to be on the "safe side" and included it, but it is not 100%
clear to me that it is really required.

Is there someone who sees the rationale behind this?
I believe that volatile means "every time you want to know the value, 
you must look to the memory", but it IMO doesn't matter if we look at
whole or part.

I've hypotetized that this may be used by some weird port for MMIO or so.

More to come after exam :)
Honza
> 
> Thanks,
> 
> --
> Mark Mitchell                   mark@codesourcery.com
> CodeSourcery, LLC               http://www.codesourcery.com

Re: simplify_subreg issues

[Florian Weimer]

Geoff Keating <geoffk@geoffk.org> writes:

> The underlying problem is that 'volatile' doesn't make sense in
> conjunction with 'complex'.  

Why do you think so?

> We can't do complex loads or stores.

The volatile qualifier has not much to do with atomicity of access, at
least according to the C standard.  For example, the C standard
requires that accesses to volatile objects are not reordered accross
sequence points.

-- 
Florian Weimer 	                  Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

Re: simplify_subreg issues

[Jan Hubicka]

> I guess this still feels a little fragile to me -- we're calling
> a function with simplify_gen_subreg in a situation where we definitely
> do not want `subreg' -- the manual says we should have no subregs
> with a MEM in them until much later in the game.  (We're doing this
> during the initial tree->rtl conversion.)  In fact, I wonder if
> simplify_gen_subreg shouldn't abort in the case that it is given
> a MEM that it cannot simplify away.  Oh, well.
Oh well too,
[I am just testing the patch with HAVE_modexx]

there are number of details I've got while unifying the subregging code.  Take
a look at current operand_subword:

rtx
operand_subword (op, offset, validate_address, mode)
     rtx op;
     unsigned int offset;
     int validate_address;
     enum machine_mode mode;
{

  .... [sanity checking] ....

  /* Form a new MEM at the requested address.  */
  if (GET_CODE (op) == MEM)
    {
      rtx addr = plus_constant (XEXP (op, 0), (offset * UNITS_PER_WORD));
      rtx new;

      if (validate_address)
	{
	  if (reload_completed)
	    {
	      if (! strict_memory_address_p (word_mode, addr))
		return 0;
	    }
	  else
	    addr = memory_address (word_mode, addr);
	}

      new = gen_rtx_MEM (word_mode, addr);
      MEM_COPY_ATTRIBUTES (new, op);
      return new;
    }

  /* Rest can be handled by simplify_subreg.  */
  return simplify_gen_subreg (word_mode, op, mode, (offset * UNITS_PER_WORD));

As you can see, it hapilly does construct memory even for volatile types.
There are other parts of compiler (gen_highpart for instance) that does the
same, so we don't mandate the scheme of not subregging volatile memory
references anyway.

In case we will get consensus that it is correct to subreg volatile memory
references of mode we can't move atomically, I will need to update these
beasts too.

But for instance in i386, the DImode do have move pattern, but we still
do 90% of operation "per partes", so operand_subword must work and return
part of memory, so the test is not quite correct anyway.

Ideas?
Honza

Re: simplify_subreg issues

[Jan Hubicka]

> 
> >> Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
> >> or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
> >> test should be done only in conjuction with some additional check
> >> that says `and is not a complex type'?
> >
> > The last.  Something like HAVE_mov* where '*' is the mode, I think.
> 
> OK.  I assume that we can never get to this code with a user-defined
> type, even one like `struct S { float f; float g; };', followed by
> a structure copy of an instance of S?  If we can it seems like we should 
> handle that similarly, but I think we're not very clever, and just
> make S have BLKmode.
I believe we represent "short structures" as integer modes, so the
code will hit.
> 
> I guess this still feels a little fragile to me -- we're calling
> a function with simplify_gen_subreg in a situation where we definitely
> do not want `subreg' -- the manual says we should have no subregs
See code in simplify_gen_subreg:

rtx
simplify_gen_subreg (outermode, op, innermode, byte)
     rtx op;
     unsigned int byte;
     enum machine_mode outermode, innermode;
{
  rtx new;
  /* Little bit of sanity checking.  */
  if (innermode == VOIDmode || outermode == VOIDmode
      || innermode == BLKmode || outermode == BLKmode)
    abort ();

  if (GET_MODE (op) != innermode
      && GET_MODE (op) != VOIDmode)
    abort ();

  if (byte % GET_MODE_SIZE (outermode)
      || byte >= GET_MODE_SIZE (innermode))
    abort ();

  new = simplify_subreg (outermode, op, innermode, byte);
  if (new)
    return new;

  if (GET_CODE (op) == SUBREG || GET_MODE (op) == VOIDmode)
    return NULL_RTX;

  return gen_rtx_SUBREG (outermode, op, byte);
}

The first part is catching obviously bogus calls and aborts.
The bottom part is catching bogus results - in case we are not able
to combine two SUBREGS or the operand is still VOIDmode.

Perhaps we can add the test for MEM as well and return 0.
Problem is that some callers (gen_lowpart mainly) require the
code to sucess. So operand_subword_force and friends will need to
be teached to load operand to register, that may cause infinite loops
in case MD file has expander that does move "by pieces".

About the MEMs on subregs.  I think we have two possibilities
1) allow them everywhere
2) avoid them everywhere

Maybe 2) makes sense.
Currently the do exist mostly in reload pass, but the reload
cleans them up, so it should not be too dificult to teach reload
to clean them up immediately and do not create them.

Similar is true about combine and cse, where we temporary
do create them, but never let them survive long.

But still there may be cases, where SUBREGs on mem make sense.
For instance on MEMs containing autoincrementing/decrementing
addresses, where it is not trivial to simplify the reference.

1) can be quite easy IMO, as what we currently do. All we need
is to update manual.

Honza

Re: simplify_subreg issues

[Jan Hubicka]

> 
> >> Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
> >> or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
> >> test should be done only in conjuction with some additional check
> >> that says `and is not a complex type'?
> >
> > The last.  Something like HAVE_mov* where '*' is the mode, I think.
Hi,
so here is patch that does the trick.  It fixes the testcase.

Tue Jun 12 12:07:37 CEST 2001  Jan Hubicka  <jh@suse.cz>

	* simplify-rtx.c (simplify_subreg): Allow volatile memory
	to be subregged in case we don't have move instruction.

Index: simplify-rtx.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/simplify-rtx.c,v
retrieving revision 1.60
diff -c -3 -p -r1.60 simplify-rtx.c
*** simplify-rtx.c	2001/06/11 09:30:47	1.60
--- simplify-rtx.c	2001/06/12 10:07:13
*************** simplify_subreg (outermode, op, innermod
*** 2409,2415 ****
  
    if (GET_CODE (op) == MEM
        && ! mode_dependent_address_p (XEXP (op, 0))
!       && ! MEM_VOLATILE_P (op)
        && GET_MODE_SIZE (outermode) <= GET_MODE_SIZE (GET_MODE (op)))
      {
        rtx new;
--- 2409,2419 ----
  
    if (GET_CODE (op) == MEM
        && ! mode_dependent_address_p (XEXP (op, 0))
!       /* Allow splitting of volatile memory references in case we don't
!          have instruction to move the whole thing.  */
!       && (! MEM_VOLATILE_P (op)
! 	  || (mov_optab->handlers[(int) innermode].insn_code
! 	      == CODE_FOR_nothing))
        && GET_MODE_SIZE (outermode) <= GET_MODE_SIZE (GET_MODE (op)))
      {
        rtx new;

Re: simplify_subreg issues

[Mark Mitchell]

> so here is patch that does the trick.  It fixes the testcase.

Approved for mainline, if it bootstraps and tests OK.

Thanks,

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: simplify_subreg issues

[Carlo Wood]

On Mon, Jun 11, 2001 at 07:56:16PM -0700, Geoff Keating wrote:
> The underlying problem is that 'volatile' doesn't make sense in
> conjunction with 'complex'.  We can't do complex loads or stores.

If I understand it correctly, then 'volatile' only makes sense for
memory references, so

TYPE volatile i;

makes no sense.  Only

TYPE volatile* ptr;

makes sense.

Secondly, complex only makes sense on non-pointers (and not even in all
cases), so

TYPE* complex ptr;

makes no sense.  Only

complex TYPE i;

makes sense.

Nevertheless, I can easily combine these two:

complex TYPE volatile* ptr;

makes sense imho.  And then I don't understand your remark anymore :/

-- 
Carlo Wood <carlo@alinoe.com>

Re: simplify_subreg issues

[Geoff Keating]

> Date: Tue, 12 Jun 2001 01:34:14 -0700
> From: Mark Mitchell <mark@codesourcery.com>
> cc: "kenner@vlsi1.ultra.nyu.edu" <kenner@vlsi1.ultra.nyu.edu>,
>         "jh@suse.cz" <jh@suse.cz>, "gcc@gcc.gnu.org" <gcc@gcc.gnu.org>
> Content-Disposition: inline
> 
> 
> >> Are you saying that the use of `MEM_VOLATILE_P' is unncessary here,
> >> or that the MEM should nont be volatile, or that the MEM_VOLATILE_P
> >> test should be done only in conjuction with some additional check
> >> that says `and is not a complex type'?
> >
> > The last.  Something like HAVE_mov* where '*' is the mode, I think.
> 
> OK.  I assume that we can never get to this code with a user-defined
> type, even one like `struct S { float f; float g; };', followed by
> a structure copy of an instance of S?  If we can it seems like we should 
> handle that similarly, but I think we're not very clever, and just
> make S have BLKmode.
> 
> I guess this still feels a little fragile to me -- we're calling
> a function with simplify_gen_subreg in a situation where we definitely
> do not want `subreg' -- the manual says we should have no subregs
> with a MEM in them until much later in the game.  (We're doing this
> during the initial tree->rtl conversion.)  In fact, I wonder if
> simplify_gen_subreg shouldn't abort in the case that it is given
> a MEM that it cannot simplify away.  Oh, well.
> 
> What about just checking to see if the mode is one of the complex
> modes, and ignoring MEM_VOLATILE_P in that case?

I seem to remember some port that did actually have complex mode move
instructions.  In that case, you'd want to do

(set (reg:SC foo) (mem:SC ...))
(set (subreg:SF (reg:SC foo) 0)  whatever)
(set (mem:SC ...) (reg:SC foo))

Think here of a hardware register, which is sensitive to 8-byte or
4-byte accesses.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: simplify_subreg issues

[Jan Hubicka]

> I seem to remember some port that did actually have complex mode move
> instructions.  In that case, you'd want to do
In mid term I am thinking about idea of adding complex arithmetics
patterns to i386 machine description using the SSE packets...

Honza

Re: simplify_subreg issues

[Jose Eugenio Marchesi]

_

Re: simplify_subreg issues

[Mark Mitchell]

> Think here of a hardware register, which is sensitive to 8-byte or
> 4-byte accesses.

Yeah -- but not a lot of hardware has registers that naturally store 
complex numbers.

I wonder if we shouldn't just say that GCC makes no guarantees about the 
number of
times something is accessed unless it is volatile and of one of the builtin 
C integer
types.  In pratice, that's what people usually care about.  It might be 
better to
make a limited promise that we could implement with confidence.

--
Mark Mitchelll               mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: simplify_subreg issues

[Geoff Keating]

> Date: Wed, 20 Jun 2001 15:03:47 -0700
> From: Mark Mitchell <mark@codesourcery.com>
> cc: "kenner@vlsi1.ultra.nyu.edu" <kenner@vlsi1.ultra.nyu.edu>,
>         "jh@suse.cz" <jh@suse.cz>, "gcc@gcc.gnu.org" <gcc@gcc.gnu.org>
> Content-Disposition: inline
> 
> 
> > Think here of a hardware register, which is sensitive to 8-byte or
> > 4-byte accesses.
> 
> Yeah -- but not a lot of hardware has registers that naturally store 
> complex numbers.
> 
> I wonder if we shouldn't just say that GCC makes no guarantees about the 
> number of
> times something is accessed unless it is volatile and of one of the builtin 
> C integer
> types.  In pratice, that's what people usually care about.  It might be 
> better to
> make a limited promise that we could implement with confidence.

That sounds like a good idea.  We could then _document_ what we think
'volatile' should mean!

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: simplify_subreg issues

[Carlo Wood]

On Tue, Jun 12, 2001 at 05:25:49AM -0400, Richard Kenner wrote:
>     The underlying problem is that 'volatile' doesn't make sense in
>     conjunction with 'complex'.  
> 
> Sure it does.
> 
>     We can't do complex loads or stores.  
> 
> That's true, but only part of what 'volatile' means.  Its *primary* meaning
> is that something else might change the value so it must be refetched every
> time and that certainly applies to 'complex'.  It is the *secondary* meaning
> of 'volatile' that's getting us into trouble here, namely the part that says
> we can't change the width of the object.
> 
> I'd say that simplify_subreg should be changed to allow the SUBREG even if
> volatile if the machine doesn't have a load insn in the input mode.

Ah, now I get it.
So when in the future(?) 64bit cpu's can load/store complex types that exist
of twice 32bit ints, this combination is valid.

-- 
Carlo Wood <carlo@alinoe.com>

Re: simplify_subreg issues

[Richard Kenner]

    Tue Jun 12 12:07:37 CEST 2001  Jan Hubicka  <jh@suse.cz>

	* simplify-rtx.c (simplify_subreg): Allow volatile memory
	to be subregged in case we don't have move instruction.

This is fine, except that I'd prefer you not verb that noun above.
So "Allow SUBREG for volatile memory when we don't have move insn."

Re: simplify_subreg issues

[Richard Kenner]

    The underlying problem is that 'volatile' doesn't make sense in
    conjunction with 'complex'.  

Sure it does.

    We can't do complex loads or stores.  

That's true, but only part of what 'volatile' means.  Its *primary* meaning
is that something else might change the value so it must be refetched every
time and that certainly applies to 'complex'.  It is the *secondary* meaning
of 'volatile' that's getting us into trouble here, namely the part that says
we can't change the width of the object.

I'd say that simplify_subreg should be changed to allow the SUBREG even if
volatile if the machine doesn't have a load insn in the input mode.