what's required for builtin_memcpy to be inlined

what's required for builtin_memcpy to be inlined

[Artur Skawina]

..other than constant size argument?

i used to have memcpy defined as

#define memcpy(t, f, n) \
(__builtin_constant_p(n) ? \
 __builtin_memcpy((t),(f),(n)) : \
 __memcpy((t),(f),(n)))

but this is apparently not enough, as it breaks for this construct
(from linux-2.3.17/fs/udf/namei.c):

> 		len = sizeof(struct FileIdentDesc);
> 
> 		if (len <= offset)
> 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);


[gcc 2.95 19990716]

Re: what's required for builtin_memcpy to be inlined

[Joe Buck]

> 
> ..other than constant size argument?

Actually, it's platform-dependent.  Presumably you have ia32 hardware.

> i used to have memcpy defined as
> 
> #define memcpy(t, f, n) \
> (__builtin_constant_p(n) ? \
>  __builtin_memcpy((t),(f),(n)) : \
>  __memcpy((t),(f),(n)))
> 
> but this is apparently not enough, as it breaks for this construct
> (from linux-2.3.17/fs/udf/namei.c):
> 
> > 		len = sizeof(struct FileIdentDesc);
> > 
> > 		if (len <= offset)
> > 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);

__builtin_constant_p(len) is false here.

You could try

#define LEN_FILEIDENT sizeof(struct FileIdentDesc)

	if (LEN_FILEIDENT <= offset)
	memcpy((Uint8 *)sfi, (Uint8 *)cfi, LEN_FILEIDENT);

Re: what's required for builtin_memcpy to be inlined

[Martin v. Loewis]

> ..other than constant size argument?
> 
> i used to have memcpy defined as
> 
> #define memcpy(t, f, n) \
> (__builtin_constant_p(n) ? \
>  __builtin_memcpy((t),(f),(n)) : \
>  __memcpy((t),(f),(n)))
> 
> but this is apparently not enough, as it breaks for this construct
> (from linux-2.3.17/fs/udf/namei.c):
> 
> > 		len = sizeof(struct FileIdentDesc);
> > 
> > 		if (len <= offset)
> > 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);

The size is not constant, it is a variable. If you write 

             memcpy((Uint8 *)sfi, (Uint8 *)cfi, sizeof(struct FileIdentDesc));

instead (or make len a const int), it should work fine.

Hope this helps,
Martin

Re: what's required for builtin_memcpy to be inlined

[Artur Skawina]

Joe Buck wrote:
> 
> > ..other than constant size argument?
> 
> Actually, it's platform-dependent.  Presumably you have ia32 hardware.

yes, forgot to mention that.

> > i used to have memcpy defined as
> >
> > #define memcpy(t, f, n) \
> > (__builtin_constant_p(n) ? \
> >  __builtin_memcpy((t),(f),(n)) : \
> >  __memcpy((t),(f),(n)))
> >
> > but this is apparently not enough, as it breaks for this construct
> > (from linux-2.3.17/fs/udf/namei.c):
> >
> > >             len = sizeof(struct FileIdentDesc);
> > >
> > >             if (len <= offset)
> > >                     memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);
> 
> __builtin_constant_p(len) is false here.

are you saying it _should_ be false?
It is _true_, which actually surprised me as i've seen it return false
for very similar code... As 'len' is obviously constant when memcpy is
called i think it is right returning true, and it's the builtin
falling back to the (nonexistent) lib version that's the real problem.
Note that the compiler generates a 'pushl  $0x26' when calling memcpy
so i just don't see why it chooses to not inline..


> You could try
> 
> #define LEN_FILEIDENT sizeof(struct FileIdentDesc)
> 
>         if (LEN_FILEIDENT <= offset)
>         memcpy((Uint8 *)sfi, (Uint8 *)cfi, LEN_FILEIDENT);

Once the problem is identified, the workaround is obvious, yes.
But i'd like to avoid doing it this way because it makes the
code less readable (though this matters more in other places than here,
and there i usually had the opposite problem -- expressions are
not recognized as constant even when that's clearly the case).

Re: what's required for builtin_memcpy to be inlined

[Jamie Lokier]

Martin v. Loewis wrote:
> The size is not constant, it is a variable. If you write 
> 
>           memcpy((Uint8 *)sfi, (Uint8 *)cfi, sizeof(struct FileIdentDesc));

The size is variable, but known by dataflow inference at compile time.
I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

-- Jamie

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Fri, Sep 10, 1999 at 05:44:26PM +0200, Jamie Lokier wrote:
> The size is variable, but known by dataflow inference at compile time.
> I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

If it can -- we currently reduce __builtin_constant_p before
gcse, so we'll not see a value set from a constant in another
basic block, as the original example had.


r~

Re: what's required for builtin_memcpy to be inlined

[Martin v. Loewis]

> The size is variable, but known by dataflow inference at compile time.
> I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

It will do so only under optimization.

Martin

Re: what's required for builtin_memcpy to be inlined

[Artur Skawina]

Richard Henderson wrote:
> 
> On Fri, Sep 10, 1999 at 05:44:26PM +0200, Jamie Lokier wrote:
> > The size is variable, but known by dataflow inference at compile time.
> > I think GCC returns 1 from __builtin_constant_p in this case, nowadays.
> 
> If it can -- we currently reduce __builtin_constant_p before
> gcse, so we'll not see a value set from a constant in another
> basic block, as the original example had.

My original "example" was cut&pasted from linux kernel sources, and
I noticed this problem because it prevented the kernel from building.

[I'm not complaing that __builtin_constant_p got smarter -- that's
 a very good thing in itself, and I hope it gets even smarter. It's
 just that a __builtin_constant_p that is smarter then a builtin
 string routine makes the latter much less useful...]

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Fri, Sep 10, 1999 at 10:51:25PM +0200, Artur Skawina wrote:
> [I'm not complaing that __builtin_constant_p got smarter -- that's
>  a very good thing in itself, and I hope it gets even smarter. It's
>  just that a __builtin_constant_p that is smarter then a builtin
>  string routine makes the latter much less useful...]

Perhaps in the moderately near future we'll be able to do 
something about this.  Nothing near-term though.


r~

Re: what's required for builtin_memcpy to be inlined

[Joern Rennecke]

> On Fri, Sep 10, 1999 at 10:51:25PM +0200, Artur Skawina wrote:
> > [I'm not complaing that __builtin_constant_p got smarter -- that's
> >  a very good thing in itself, and I hope it gets even smarter. It's
> >  just that a __builtin_constant_p that is smarter then a builtin
> >  string routine makes the latter much less useful...]
> 
> Perhaps in the moderately near future we'll be able to do 
> something about this.  Nothing near-term though.

Well, wouldn't this depend on code contributions?
I think it should be faily straightforward to fold builtin function calls
when their arguments are constant.

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Mon, Sep 13, 1999 at 06:28:08PM +0100, Joern Rennecke wrote:
> I think it should be faily straightforward to fold builtin function calls
> when their arguments are constant.

And we do do that.  What he wants is to delay the expansion of
__builtin_memcpy until after global constant propogation.


r~

Re: what's required for builtin_memcpy to be inlined

[Joern Rennecke]

> And we do do that.  What he wants is to delay the expansion of
> __builtin_memcpy until after global constant propogation.

We do that only at expansion time now.  But we could also do it when
we have folded an argument to a builtin function that is coded as a
call_insn so far.

what's required for builtin_memcpy to be inlined

[Artur Skawina]

..other than constant size argument?

i used to have memcpy defined as

#define memcpy(t, f, n) \
(__builtin_constant_p(n) ? \
 __builtin_memcpy((t),(f),(n)) : \
 __memcpy((t),(f),(n)))

but this is apparently not enough, as it breaks for this construct
(from linux-2.3.17/fs/udf/namei.c):

> 		len = sizeof(struct FileIdentDesc);
> 
> 		if (len <= offset)
> 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);


[gcc 2.95 19990716]

Re: what's required for builtin_memcpy to be inlined

[Martin v. Loewis]

> ..other than constant size argument?
> 
> i used to have memcpy defined as
> 
> #define memcpy(t, f, n) \
> (__builtin_constant_p(n) ? \
>  __builtin_memcpy((t),(f),(n)) : \
>  __memcpy((t),(f),(n)))
> 
> but this is apparently not enough, as it breaks for this construct
> (from linux-2.3.17/fs/udf/namei.c):
> 
> > 		len = sizeof(struct FileIdentDesc);
> > 
> > 		if (len <= offset)
> > 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);

The size is not constant, it is a variable. If you write 

             memcpy((Uint8 *)sfi, (Uint8 *)cfi, sizeof(struct FileIdentDesc));

instead (or make len a const int), it should work fine.

Hope this helps,
Martin

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Mon, Sep 13, 1999 at 06:28:08PM +0100, Joern Rennecke wrote:
> I think it should be faily straightforward to fold builtin function calls
> when their arguments are constant.

And we do do that.  What he wants is to delay the expansion of
__builtin_memcpy until after global constant propogation.


r~

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Fri, Sep 10, 1999 at 10:51:25PM +0200, Artur Skawina wrote:
> [I'm not complaing that __builtin_constant_p got smarter -- that's
>  a very good thing in itself, and I hope it gets even smarter. It's
>  just that a __builtin_constant_p that is smarter then a builtin
>  string routine makes the latter much less useful...]

Perhaps in the moderately near future we'll be able to do 
something about this.  Nothing near-term though.


r~

Re: what's required for builtin_memcpy to be inlined

[Artur Skawina]

Joe Buck wrote:
> 
> > ..other than constant size argument?
> 
> Actually, it's platform-dependent.  Presumably you have ia32 hardware.

yes, forgot to mention that.

> > i used to have memcpy defined as
> >
> > #define memcpy(t, f, n) \
> > (__builtin_constant_p(n) ? \
> >  __builtin_memcpy((t),(f),(n)) : \
> >  __memcpy((t),(f),(n)))
> >
> > but this is apparently not enough, as it breaks for this construct
> > (from linux-2.3.17/fs/udf/namei.c):
> >
> > >             len = sizeof(struct FileIdentDesc);
> > >
> > >             if (len <= offset)
> > >                     memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);
> 
> __builtin_constant_p(len) is false here.

are you saying it _should_ be false?
It is _true_, which actually surprised me as i've seen it return false
for very similar code... As 'len' is obviously constant when memcpy is
called i think it is right returning true, and it's the builtin
falling back to the (nonexistent) lib version that's the real problem.
Note that the compiler generates a 'pushl  $0x26' when calling memcpy
so i just don't see why it chooses to not inline..


> You could try
> 
> #define LEN_FILEIDENT sizeof(struct FileIdentDesc)
> 
>         if (LEN_FILEIDENT <= offset)
>         memcpy((Uint8 *)sfi, (Uint8 *)cfi, LEN_FILEIDENT);

Once the problem is identified, the workaround is obvious, yes.
But i'd like to avoid doing it this way because it makes the
code less readable (though this matters more in other places than here,
and there i usually had the opposite problem -- expressions are
not recognized as constant even when that's clearly the case).

Re: what's required for builtin_memcpy to be inlined

[Joe Buck]

> 
> ..other than constant size argument?

Actually, it's platform-dependent.  Presumably you have ia32 hardware.

> i used to have memcpy defined as
> 
> #define memcpy(t, f, n) \
> (__builtin_constant_p(n) ? \
>  __builtin_memcpy((t),(f),(n)) : \
>  __memcpy((t),(f),(n)))
> 
> but this is apparently not enough, as it breaks for this construct
> (from linux-2.3.17/fs/udf/namei.c):
> 
> > 		len = sizeof(struct FileIdentDesc);
> > 
> > 		if (len <= offset)
> > 			memcpy((Uint8 *)sfi, (Uint8 *)cfi, len);

__builtin_constant_p(len) is false here.

You could try

#define LEN_FILEIDENT sizeof(struct FileIdentDesc)

	if (LEN_FILEIDENT <= offset)
	memcpy((Uint8 *)sfi, (Uint8 *)cfi, LEN_FILEIDENT);

Re: what's required for builtin_memcpy to be inlined

[Joern Rennecke]

> And we do do that.  What he wants is to delay the expansion of
> __builtin_memcpy until after global constant propogation.

We do that only at expansion time now.  But we could also do it when
we have folded an argument to a builtin function that is coded as a
call_insn so far.

Re: what's required for builtin_memcpy to be inlined

[Richard Henderson]

On Fri, Sep 10, 1999 at 05:44:26PM +0200, Jamie Lokier wrote:
> The size is variable, but known by dataflow inference at compile time.
> I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

If it can -- we currently reduce __builtin_constant_p before
gcse, so we'll not see a value set from a constant in another
basic block, as the original example had.


r~

Re: what's required for builtin_memcpy to be inlined

[Joern Rennecke]

> On Fri, Sep 10, 1999 at 10:51:25PM +0200, Artur Skawina wrote:
> > [I'm not complaing that __builtin_constant_p got smarter -- that's
> >  a very good thing in itself, and I hope it gets even smarter. It's
> >  just that a __builtin_constant_p that is smarter then a builtin
> >  string routine makes the latter much less useful...]
> 
> Perhaps in the moderately near future we'll be able to do 
> something about this.  Nothing near-term though.

Well, wouldn't this depend on code contributions?
I think it should be faily straightforward to fold builtin function calls
when their arguments are constant.

Re: what's required for builtin_memcpy to be inlined

[Artur Skawina]

Richard Henderson wrote:
> 
> On Fri, Sep 10, 1999 at 05:44:26PM +0200, Jamie Lokier wrote:
> > The size is variable, but known by dataflow inference at compile time.
> > I think GCC returns 1 from __builtin_constant_p in this case, nowadays.
> 
> If it can -- we currently reduce __builtin_constant_p before
> gcse, so we'll not see a value set from a constant in another
> basic block, as the original example had.

My original "example" was cut&pasted from linux kernel sources, and
I noticed this problem because it prevented the kernel from building.

[I'm not complaing that __builtin_constant_p got smarter -- that's
 a very good thing in itself, and I hope it gets even smarter. It's
 just that a __builtin_constant_p that is smarter then a builtin
 string routine makes the latter much less useful...]

Re: what's required for builtin_memcpy to be inlined

[Martin v. Loewis]

> The size is variable, but known by dataflow inference at compile time.
> I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

It will do so only under optimization.

Martin

Re: what's required for builtin_memcpy to be inlined

[Jamie Lokier]

Martin v. Loewis wrote:
> The size is not constant, it is a variable. If you write 
> 
>           memcpy((Uint8 *)sfi, (Uint8 *)cfi, sizeof(struct FileIdentDesc));

The size is variable, but known by dataflow inference at compile time.
I think GCC returns 1 from __builtin_constant_p in this case, nowadays.

-- Jamie