[PATCH] D88712: [CGBuiltin] Respect asm labels and redefine_extname for builtins with specialized emitting

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a subscriber: zatrazz.
MaskRay added a comment.

In D88712#2333030 <https://reviews.llvm.org/D88712#2333030>, @rsmith wrote:

> I worry that we're chasing after the implementation details of GCC here. It seems self-contradictory to say all three of these things at once:
>
> 1. The frontend function `foo` has known, builtin semantics X. (Eg, we'll use those semantics in the frontend.)
> 2. The symbol `foo` has known, builtin semantics X. (Eg, we'll synthesize calls to `foo` from the middle-end.)
> 3. It's not correct to lower a call to the frontend function `foo` to the symbol `foo`.
>
> So, from a principled standpoint, which of the above do we not consider to be correct in this situation?
>
> - If we don't consider (1) to be correct, then we need to stop treating `foo` as a builtin everywhere -- we shouldn't be constant-evaluating calls to it, in particular. That's problematic in principle, because the `asm` label might be added after we've already seen the first declaration and added a `BuiltinAttr` to it. If we want to go in this direction, I think we should require a build that attaches an `asm` label to a builtin to also pass `-fno-builtin-foo` rather than trying to un-builtin a builtin function after the fact.
> - If we don't consider (2) to be correct, then we need to stop the middle-end from inserting calls to the symbol `foo`, and get it to use the new symbol instead. That'd be the "teach the target libcall info about this" approach, which (as you point out) would be quite complex.
> - The status quo is that we don't consider (3) to be correct. If we take this path, I suppose we could say that we make a best effort to use the renamed symbol, but provide no guarantees. That seems unprincipled and will likely continue to cause problems down the line, but maybe this gets us closest to the observed GCC behavior?

It took me quite a while to understand the "take 2 out of the 3 guarantees" theorem;-)

I think people do want to keep (1) (if it is profitable to expand a memcpy, do it). This also means that people do not want to add -fno-builtin-memcpy.

People do want (3): that is why they use an `asm("__GI_memcpy")` in the first place.
The status quo is "we don't consider (3) to be correct." -> "we ignore asm("__GI_memcpy")". This makes the system consistent but it is unexpected.

So unfortunately we are making a compromise on (2): refuting it is difficult in both GCC and Clang.
For most libcalls, compilers don't generate them, so it is a small loss. For the few glibc cares about, it uses the `asm("memcpy = __GI_memcpy");` GNU as feature to make the second level redirection.
D88625 <https://reviews.llvm.org/D88625> will do it on MC side.

We still have another choice: don't use `memcpy`, use `__memcpy` instead. However, that will be a hard hammer and according to @zatrazz people will complain 'with gcc this works as intended'...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88712/new/

https://reviews.llvm.org/D88712