LLVM fails to codegenerate functions whose name matches an llvm.<builtin>

[echristo]

Bugzilla Link	23953
Resolution	FIXED
Resolved on	Nov 05, 2015 12:54
Version	trunk
OS	Linux
Blocks	#24719
CC	@majnemer,@dwblaikie,@zygoloid

[echristo]

assigned to @majnemer

[echristo]

Testcase:

dzur:~/tmp> cat bar.c
int t;

extern inline void attribute((gnu_inline))
memcpy(void *a, const void *restrict x, unsigned long b) {
__builtin_memcpy((a), x, b);
}

int main() {
memcpy((void *)(&t), &t, 1);
return 0;
}

things that also fail are prefetch, etc:

extern inline void
attribute((gnu_inline))
prefetch(const void *restrict x)
{
__builtin_prefetch ((x), 0, 1);
}

int t;

int main() {
prefetch((void *)(&t));
return 0;
}

both extern inline and gnu_inline appear to be necessary here.

[majnemer]

this testcase also fails:
__inline attribute((always_inline))
int abs(int j) {
return __builtin_abs(j);
}

int main() {
return abs(0);
}

The issue is that we believe the function is trivially recursive:

1. we call clang::CodeGen::CodeGenModule::EmitGlobalDefinition
2. this calls clang::CodeGen::CodeGenModule::shouldEmitFunction
3. this calls clang::CodeGen::CodeGenModule::isTriviallyRecursive

isTriviallyRecursive looks for a call to __builtin_abs (or __builtin_prefetch in the original example).

Perhaps we should only bother running isTriviallyRecursive if the function is not marked always_inline?

[dwblaikie]

What's the proper handling for defining a built in (not sure what the right term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them
2. assume they don't exist and that this TU is defining them - in which case we shouldn't be trying to treat them as the builtin form we would provide in (1)

no?

[majnemer]

What's the proper handling for defining a built in (not sure what the right
term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them

What happens if the call is coming from inside the house: you are the implementation and are trying to provide a definition.

2. assume they don't exist and that this TU is defining them - in which case
   we shouldn't be trying to treat them as the builtin form we would provide in
   (1)

no?

What if your definition needs to massage the LLVM builtin? It wouldn't be appropriate to emit a call to the builtin in this case.

[dwblaikie]

What's the proper handling for defining a built in (not sure what the right
term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them

What happens if the call is coming from inside the house: you are the
implementation and are trying to provide a definition.

Sorry, that's what I was trying to say - I would've thought/hoped/desired that the compiler has an explicit awareness of these two states (either you're the implementation or you're using the implementation - if the compiler is compiling the implementation then it shouldn't assume it knows anything about the "abs" function - if it is compiling a usage, it should error on an attempt to define the "abs" function)

But if we don't already have that explicit awareness - yeah, we just have to muddle through. If we're muddling through, it still seems weird to rely on always_inline to fix this behavior (but I know very little of this) - shouldn't we be aware that this is a definition of 'abs' so it's got special dispensation to be distinct from __buitin_abs? Not sure - maybe should just talk to you over a drink or something

2. assume they don't exist and that this TU is defining them - in which case
   we shouldn't be trying to treat them as the builtin form we would provide in
   (1)

no?

What if your definition needs to massage the LLVM builtin? It wouldn't be
appropriate to emit a call to the builtin in this case.

Not sure what you mean by "massage the LLVM builtin" in this context.

[majnemer]

What's the proper handling for defining a built in (not sure what the right
term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them

What happens if the call is coming from inside the house: you are the
implementation and are trying to provide a definition.

Sorry, that's what I was trying to say - I would've thought/hoped/desired
that the compiler has an explicit awareness of these two states (either
you're the implementation or you're using the implementation - if the
compiler is compiling the implementation then it shouldn't assume it knows
anything about the "abs" function - if it is compiling a usage, it should
error on an attempt to define the "abs" function)

But if we don't already have that explicit awareness - yeah, we just have to
muddle through. If we're muddling through, it still seems weird to rely on
always_inline to fix this behavior (but I know very little of this) -
shouldn't we be aware that this is a definition of 'abs' so it's got special
dispensation to be distinct from __buitin_abs? Not sure - maybe should just
talk to you over a drink or something

2. assume they don't exist and that this TU is defining them - in which case
   we shouldn't be trying to treat them as the builtin form we would provide in
   (1)

no?

What if your definition needs to massage the LLVM builtin? It wouldn't be
appropriate to emit a call to the builtin in this case.

Not sure what you mean by "massage the LLVM builtin" in this context.

Imagine a builtin called 'foo' and it's builtin, '__builtin_foo'. Suppose we'd like to define 'foo' as such:

return foo(int a, int b) {
__builtin_foo(b, a);
}

In this case, the builtin does not precisely match the arguments because it permutes them. We cannot simply call the builtin.

[dwblaikie]

What's the proper handling for defining a built in (not sure what the right
term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them

What happens if the call is coming from inside the house: you are the
implementation and are trying to provide a definition.

Sorry, that's what I was trying to say - I would've thought/hoped/desired
that the compiler has an explicit awareness of these two states (either
you're the implementation or you're using the implementation - if the
compiler is compiling the implementation then it shouldn't assume it knows
anything about the "abs" function - if it is compiling a usage, it should
error on an attempt to define the "abs" function)

But if we don't already have that explicit awareness - yeah, we just have to
muddle through. If we're muddling through, it still seems weird to rely on
always_inline to fix this behavior (but I know very little of this) -
shouldn't we be aware that this is a definition of 'abs' so it's got special
dispensation to be distinct from __buitin_abs? Not sure - maybe should just
talk to you over a drink or something

2. assume they don't exist and that this TU is defining them - in which case
   we shouldn't be trying to treat them as the builtin form we would provide in
   (1)

no?

What if your definition needs to massage the LLVM builtin? It wouldn't be
appropriate to emit a call to the builtin in this case.

Not sure what you mean by "massage the LLVM builtin" in this context.

Imagine a builtin called 'foo' and it's builtin, '__builtin_foo'. Suppose
we'd like to define 'foo' as such:

return foo(int a, int b) {
__builtin_foo(b, a);
}

In this case, the builtin does not precisely match the arguments because it
permutes them. We cannot simply call the builtin.

Sort of follow but don't. Depending on who "we" are in those sentences.

The compiler already has a hardcoded assumption that foo(x, y) can be replaced by __builtin_foo(x, y) right? So if we're in a translation unit that defines foo, I assume we just accept that however foo is defined is OK by us. Up to the library implementation to provide equivalent semantics using our builtins or otherwise...

eh, I'm missing steps - don't really understand whether a library implementation would use the compiler builtins to define their canonical definition.

In any case - if the compiler is compiling the library implementation, I would imagine that the compiler would, for that translation unit, assume it knows nothing about the builtin - just treat it like any other function. (I would sort of hope it was built in a particular mode where it didn't have to decide on this assumption lazily - I thought that was what -fstandalone or the like were for)

[majnemer]

What's the proper handling for defining a built in (not sure what the right
term is - intrinsic?) function like abs, memcpy, etc?

I imagine we should be either in one of two modes:

1. assume these things already exist and reject attempts to define them

What happens if the call is coming from inside the house: you are the
implementation and are trying to provide a definition.

Sorry, that's what I was trying to say - I would've thought/hoped/desired
that the compiler has an explicit awareness of these two states (either
you're the implementation or you're using the implementation - if the
compiler is compiling the implementation then it shouldn't assume it knows
anything about the "abs" function - if it is compiling a usage, it should
error on an attempt to define the "abs" function)

But if we don't already have that explicit awareness - yeah, we just have to
muddle through. If we're muddling through, it still seems weird to rely on
always_inline to fix this behavior (but I know very little of this) -
shouldn't we be aware that this is a definition of 'abs' so it's got special
dispensation to be distinct from __buitin_abs? Not sure - maybe should just
talk to you over a drink or something

2. assume they don't exist and that this TU is defining them - in which case
   we shouldn't be trying to treat them as the builtin form we would provide in
   (1)

no?

What if your definition needs to massage the LLVM builtin? It wouldn't be
appropriate to emit a call to the builtin in this case.

Not sure what you mean by "massage the LLVM builtin" in this context.

Imagine a builtin called 'foo' and it's builtin, '__builtin_foo'. Suppose
we'd like to define 'foo' as such:

return foo(int a, int b) {
__builtin_foo(b, a);
}

In this case, the builtin does not precisely match the arguments because it
permutes them. We cannot simply call the builtin.

Sort of follow but don't. Depending on who "we" are in those sentences.

The compiler already has a hardcoded assumption that foo(x, y) can be
replaced by __builtin_foo(x, y) right? So if we're in a translation unit
that defines foo, I assume we just accept that however foo is defined is OK
by us. Up to the library implementation to provide equivalent semantics
using our builtins or otherwise...

eh, I'm missing steps - don't really understand whether a library
implementation would use the compiler builtins to define their canonical
definition.

In any case - if the compiler is compiling the library implementation, I
would imagine that the compiler would, for that translation unit, assume it
knows nothing about the builtin - just treat it like any other function. (I
would sort of hope it was built in a particular mode where it didn't have to
decide on this assumption lazily - I thought that was what -fstandalone or
the like were for)

Just because we decided to name a builtin __builtin_prefetch does not give us the right to dictate the prototype (and internal mechanics) of a function called prefetch as prefetch is not specified by any standard.

[dwblaikie]

Just because we decided to name a builtin __builtin_prefetch does not give
us the right to dictate the prototype (and internal mechanics) of a function
called prefetch as prefetch is not specified by any standard.

OK, maybe this is where I got off the rails. If we have no ownership over the function called "prefetch" then why would we consider it to be trivially recursive? Why would the fix to this have anything to do with always_inlineness of such a function? Shouldn't we just be more agnostic to functions named "prefetch"? (or any other suffix of one of our builtins)

[majnemer]

Fixed in r240735.

[llvmbot]

mentioned in issue #24719