"+m" constraints bogus?

"+m" constraints bogus?

[Jan Hubicka]

Hi,
The following bit of documentation:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [3/8] i386: bitops: Rectify bogus "+m" constraints
> 
> >From the gcc manual:
> 
>   Extended asm supports input-output or read-write operands. Use the
>   constraint character `+' to indicate such an operand and list it with
>   the output operands. You should only use read-write operands when the
>   constraints for the operand (or the operand in which only some of the
>   bits are to be changed) allow a register.
> 
> So, using the "+" constraint modifier for memory, like "+m" is bogus.
> We must simply specify "=m" which handles the case correctly.

seems to instruct people to use just output constraint for memory
references that are read/write:
> @@ -42,7 +42,7 @@ static inline void set_bit(int nr, volatile unsigned long * addr)
>  {
>  	__asm__ __volatile__( LOCK_PREFIX
>  		"btsl %1,%0"
> -		:"+m" (ADDR)
> +		:"=m" (ADDR)
>  		:"r" (nr));
>  }

Linux kernel is using input/output memory constraints for a while and I
am not quite sure why the documentation above insist on a register.
Obviously having input/output constant is bogus, but for memory the
meaning is clear.  We used to have problems with reload producing
different memory address for input and output variant resulting in ICE,
but I believe this problem has been fixed for a while.  What about
updating docs like this?

Honza

Index: extend.texi
===================================================================
*** extend.texi	(revision 126800)
--- extend.texi	(working copy)
*************** operands.  Use the constraint character 
*** 4224,4230 ****
  operand and list it with the output operands.  You should only use
  read-write operands when the constraints for the operand (or the
  operand in which only some of the bits are to be changed) allow a
! register.
  
  You may, as an alternative, logically split its function into two
  separate operands, one input operand and one write-only output
--- 4224,4230 ----
  operand and list it with the output operands.  You should only use
  read-write operands when the constraints for the operand (or the
  operand in which only some of the bits are to be changed) allow a
! register or memory.
  
  You may, as an alternative, logically split its function into two
  separate operands, one input operand and one write-only output

Re: "+m" constraints bogus?

[Michael Matz]

Hi,

On Mon, 23 Jul 2007, Jan Hubicka wrote:

> >   Extended asm supports input-output or read-write operands. Use the 
> >   constraint character `+' to indicate such an operand and list it 
> >   with the output operands. You should only use read-write operands 
> >   when the constraints for the operand (or the operand in which only 
> >   some of the bits are to be changed) allow a register.
> > 
> > So, using the "+" constraint modifier for memory, like "+m" is bogus. 
> > We must simply specify "=m" which handles the case correctly.
> 
> seems to instruct people to use just output constraint for memory 
> references that are read/write:
> > @@ -42,7 +42,7 @@ static inline void set_bit(int nr, volatile unsigned long * addr)
> >  {
> >  	__asm__ __volatile__( LOCK_PREFIX
> >  		"btsl %1,%0"
> > -		:"+m" (ADDR)
> > +		:"=m" (ADDR)
> >  		:"r" (nr));
> >  }
> 
> Linux kernel is using input/output memory constraints for a while and I
> am not quite sure why the documentation above insist on a register.

It says that because of the problems you described in reload.  Are you 
sure those were ever fixed? 

> Obviously having input/output constant is bogus, but for memory the
> meaning is clear.  We used to have problems with reload producing
> different memory address for input and output variant resulting in ICE,
> but I believe this problem has been fixed for a while.  What about
> updating docs like this?


Ciao,
Michael.

Re: "+m" constraints bogus?

[Jan Hubicka]

> 
> It says that because of the problems you described in reload.  Are you 
> sure those were ever fixed? 

Well, something is definitly weird.  I can find patch:
http://gcc.gnu.org/ml/gcc-patches/2003-12/msg01806.html
that added a waring.  In 3.4 the warning was taken back by rth:
http://gcc.gnu.org/ml/gcc-patches/2004-05/msg00438.html
with some disucssion that lead to nowhere.

I will try to find the original Jason's problem, does anyone recall?

Honza	
> 
> > Obviously having input/output constant is bogus, but for memory the
> > meaning is clear.  We used to have problems with reload producing
> > different memory address for input and output variant resulting in ICE,
> > but I believe this problem has been fixed for a while.  What about
> > updating docs like this?
> 
> 
> Ciao,
> Michael.

Re: "+m" constraints bogus?

[Andrew Pinski]

On 7/24/07, Jan Hubicka <jh@suse.cz> wrote:
> I will try to find the original Jason's problem, does anyone recall?

well the gimplifier no longer allows "+m" to pass to expand, we always
expoand the constraints.

Take:
int f(int a)
{
  asm("":"+m"(a) );
  return a;
}

The gimplifier produces:
apinski@debian:~$ cat t1212.c.004t.gimple
f (a)
{
  int D.1627;

  __asm__("":"=m" a:"m" a);
  D.1627 = a;
  return D.1627;
}


Thanks,
Andrew Pinski

Re: "+m" constraints bogus?

[Jan Hubicka]

> On 7/24/07, Jan Hubicka <jh@suse.cz> wrote:
> >I will try to find the original Jason's problem, does anyone recall?
> 
> well the gimplifier no longer allows "+m" to pass to expand, we always
> expoand the constraints.

Yep, I noticed we decompose it now, however it still does not quite
answer whether we can accept +m now or not... (ie whether there exists
testcases where reload desynchronize input and output constraint for
some good reason).

Honza

Re: "+m" constraints bogus?

[Michael Matz]

Hi,

On Tue, 24 Jul 2007, Jan Hubicka wrote:

> > 
> > It says that because of the problems you described in reload.  Are you 
> > sure those were ever fixed? 
> 
> Well, something is definitly weird.  I can find patch:
> http://gcc.gnu.org/ml/gcc-patches/2003-12/msg01806.html
> that added a waring.  In 3.4 the warning was taken back by rth:
> http://gcc.gnu.org/ml/gcc-patches/2004-05/msg00438.html
> with some disucssion that lead to nowhere.
> 
> I will try to find the original Jason's problem, does anyone recall?

here:
http://gcc.gnu.org/ml/gcc-patches/2003-12/msg01358.html

It describes the potential problem again, and I know of no changes in 
reload which would have magically handled matching mem-only constraints.  
I believe the problem currently only doesn't exist because the 
gimplification mentioned by Andrew doesn't let +m come through to RTL.  So 
it would probably be best to ensure that it stays that way, and maybe add 
an assert instead of the warning, that we don't see matching or inout 
constraints which don't allow registers.

For reference, the potential problem in reload is the following: matching 
constraints might result in invalid operands (address not using the same 
pseudo in our case, for instance) for which reloads are pushed.  Such 
pushed reloads can only be satisfied by a register in an appropriate class 
(the reload reg).  If the alternative doesn't allow any registers such 
reload can _never_ be satisfied --> boom.  That's the old problem of 
reload that it can't reload by using memory.


Ciao,
Michael.

Re: "+m" constraints bogus?

[Jan Hubicka]

Hi,
thanks for pointer!
> here:
> http://gcc.gnu.org/ml/gcc-patches/2003-12/msg01358.html
> 
> It describes the potential problem again, and I know of no changes in 
> reload which would have magically handled matching mem-only constraints.  
> I believe the problem currently only doesn't exist because the 
> gimplification mentioned by Andrew doesn't let +m come through to RTL.  So 
> it would probably be best to ensure that it stays that way, and maybe add 
> an assert instead of the warning, that we don't see matching or inout 
> constraints which don't allow registers.

Perhaps, we however still would need to warn about:
asm __volatile__ ("":"=m"(a):"0"(a));
(which correctly triggers the warning in question)
> 
> For reference, the potential problem in reload is the following: matching 
> constraints might result in invalid operands (address not using the same 
> pseudo in our case, for instance) for which reloads are pushed.  Such 
> pushed reloads can only be satisfied by a register in an appropriate class 
> (the reload reg).  If the alternative doesn't allow any registers such 
> reload can _never_ be satisfied --> boom.  That's the old problem of 
> reload that it can't reload by using memory.

Yep, I am aware of those problems (reload dying in horrible death as
soon as something didn't ended up matching). I was somewhat confused
thinking that gimplifier gimplifies into the pair as in my testcase
above, not the "=m" "m" pair.

I guess we are safe now support them so I would just update the manual
with a simple testcase so we know gimplifier does not break and we won't
re-start emitting the warning?

Honza
> 
> 
> Ciao,
> Michael.

Re: "+m" constraints bogus?

[Michael Matz]

Hi,

On Wed, 25 Jul 2007, Jan Hubicka wrote:

> Yep, I am aware of those problems (reload dying in horrible death as 
> soon as something didn't ended up matching). I was somewhat confused 
> thinking that gimplifier gimplifies into the pair as in my testcase 
> above, not the "=m" "m" pair.
> 
> I guess we are safe now support them so I would just update the manual 
> with a simple testcase so we know gimplifier does not break and we won't 
> re-start emitting the warning?

Seems sensible, yes.


Ciao,
Michael.

Re: "+m" constraints bogus?

[Jan Hubicka]

> Hi,
> 
> On Wed, 25 Jul 2007, Jan Hubicka wrote:
> 
> > Yep, I am aware of those problems (reload dying in horrible death as 
> > soon as something didn't ended up matching). I was somewhat confused 
> > thinking that gimplifier gimplifies into the pair as in my testcase 
> > above, not the "=m" "m" pair.
> > 
> > I guess we are safe now support them so I would just update the manual 
> > with a simple testcase so we know gimplifier does not break and we won't 
> > re-start emitting the warning?
> 
> Seems sensible, yes.

Just because it probably wasn't mentioned explicitely - the gimplifier
approach is a bit weak in a sense that there is theoretically nothing
explicitly keeping memory address of input and output operand the same
through the compilation and in future we might invent optimizer
modifying memory operands in a way that this breaks, but it seems like
sane invariant to maintain for ASM operands...

Honza
> 
> 
> Ciao,
> Michael.

Re: "+m" constraints bogus?

[Michael Matz]

Hi,

On Wed, 25 Jul 2007, Jan Hubicka wrote:

> > > I guess we are safe now support them so I would just update the 
> > > manual with a simple testcase so we know gimplifier does not break 
> > > and we won't re-start emitting the warning?
> > 
> > Seems sensible, yes.
> 
> Just because it probably wasn't mentioned explicitely - the gimplifier 
> approach is a bit weak in a sense that there is theoretically nothing 
> explicitly keeping memory address of input and output operand the same 
> through the compilation and in future we might invent optimizer 
> modifying memory operands in a way that this breaks, but it seems like 
> sane invariant to maintain for ASM operands...

For
  asm ("", "=m" (bla) : "m" (bla))'
any transformation which would make the address of both operands be not 
the same (namely &bla) would be invalid.  They might be placed into 
separate pseudo regs (or become completely different expressions, like 
"&bla -4 + 4" and "&bla -5+5") but they have to be equal at runtime.  And 
as long as that holds there's no problem, as reload is able to reload both 
operands individually (by reloading the address part).  It's just when 
they have to match that reload gets into trouble.


Ciao,
Michael.

Re: "+m" constraints bogus?

[Jan Hubicka]

> Hi,
> 
> On Wed, 25 Jul 2007, Jan Hubicka wrote:
> 
> > > > I guess we are safe now support them so I would just update the 
> > > > manual with a simple testcase so we know gimplifier does not break 
> > > > and we won't re-start emitting the warning?
> > > 
> > > Seems sensible, yes.
> > 
> > Just because it probably wasn't mentioned explicitely - the gimplifier 
> > approach is a bit weak in a sense that there is theoretically nothing 
> > explicitly keeping memory address of input and output operand the same 
> > through the compilation and in future we might invent optimizer 
> > modifying memory operands in a way that this breaks, but it seems like 
> > sane invariant to maintain for ASM operands...
> 
> For
>   asm ("", "=m" (bla) : "m" (bla))'
> any transformation which would make the address of both operands be not 
> the same (namely &bla) would be invalid.  They might be placed into 
> separate pseudo regs (or become completely different expressions, like 
> "&bla -4 + 4" and "&bla -5+5") but they have to be equal at runtime.  And 
> as long as that holds there's no problem, as reload is able to reload both 
> operands individually (by reloading the address part).  It's just when 
> they have to match that reload gets into trouble.

Well, I can imagine that in the future we might, for instance, want to
do copy propagation on memory locations as we do on register that might
result in optimizer replacing destination bla by something else
believing that the asm will read from bla and write somewhere else.
But we don't do that and many asm statements would break by such
transformation.

Honza