Skip to content

Proposal: Add never_intrinsify to std.builtin.CallModifier #21833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alexrp opened this issue Oct 27, 2024 · 10 comments · Fixed by #22154
Closed

Proposal: Add never_intrinsify to std.builtin.CallModifier #21833

alexrp opened this issue Oct 27, 2024 · 10 comments · Fixed by #22154
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@alexrp
Copy link
Member

alexrp commented Oct 27, 2024

This modifier prevents the compiler from turning a function call into an intrinsic/builtin. In other words, when you call a function using this modifier, you're guaranteed to actually get a call to that function in the generated code (note: inlining is still permitted). Concretely, it would map to the nobuiltin LLVM attribute at the call site (not to be confused with the no-builtins function attribute), and whatever equivalent exists for other backends.

Motivated by #21831 (and I suspect numerous other cases in compiler-rt if I went digging).

FWIW, Clang has this in the form of the no_builtin attribute, so I think it's important that Zig also be able to express this.

@alexrp alexrp added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Oct 27, 2024
@alexrp alexrp added this to the 0.15.0 milestone Oct 27, 2024
@rohlem
Copy link
Contributor

rohlem commented Oct 27, 2024

Is this phenomenon intrinsically linked to callsites and/or the function boundary?
I imagine the compiler might replace any code section with an equivalent builtin.
For the use case of implementing compiler_rt, where the functions map 1:1, a call modifier would be a natural fit,
but maybe a nobuiltin/nointrinsify block (similar to nosuspend blocks) would be more flexible / useful in general?

What also looks strange to me is that the linked no_builtin attribute is declared for the callee function, while a CallModifier is currently supplied at the caller's @call.
EDIT: Ah, so LLVM puts it on the caller but clang uses a callee attribute? That's weird, but maybe normal for LLVM's inconsistencies. It would still be worth picking the more sensible choice for Zig though.
For builtins only called via the compiler it might not matter, but it seems strange to me to generate a single function twice - once for never_intrinsify callers and once for callers that allow builtins.

@Rexicon226
Copy link
Contributor

For the use case of implementing compiler_rt, where the functions map 1:1, a call modifier would be a natural fit,
but maybe a nobuiltin/nointrinsify block (similar to nosuspend blocks) would be more flexible / useful in general?

Not in any order, but here would be my counter points:

  1. Just looking at the technical aspect, how would you even implement something like this?
  2. never_intrinsify is necessary at specific callsites, so I don't think it makes much sense for it to be inside of the callee.
  3. never_intrinsify would be quite rarely used, so making it a "language semantic" seems superfluous and unnecessary to me.

@rohlem
Copy link
Contributor

rohlem commented Oct 27, 2024

  1. never_intrinsify is necessary at specific callsites, so I don't think it makes much sense for it to be inside of the callee.

In my understanding (which may be wrong), f.e. in the use case of our own memcpy implementation,
that is a function (callee) we provide in compiler_rt, which we never want to be translated as a call to a builtin (no matter the caller/callsite).

The fact that the compiler is the one emitting the calls (generating callsites) makes it feasible to always specify this as a callsite-attribute,
but I don't see the point of ever allowing a call to a compiler_rt function to be replaced by a compiler intrinsic - in that case I'd expect callers use builtins like @memcpy etc. .

  1. never_intrinsify would be quite rarely used, so making it a "language semantic" seems superfluous and unnecessary to me.

Probably true, afaiu hand-written assembly already isn't affected by these builtin-replacements?
It might still be useful for some micro-optimization use cases, but those probably shouldn't be prioritized.

@alexrp
Copy link
Member Author

alexrp commented Oct 27, 2024

Is this phenomenon intrinsically linked to callsites and/or the function boundary?
I imagine the compiler might replace any code section with an equivalent builtin.
For the use case of implementing compiler_rt, where the functions map 1:1, a call modifier would be a natural fit,
but maybe a nobuiltin/nointrinsify block (similar to nosuspend blocks) would be more flexible / useful in general?

That may be true, but:

  1. I don't currently have a motivating use case for such a broad feature.
  2. It's hard to justify the added language complexity for such a niche problem.
  3. The feature can always be broadened later if needed.

I think a call modifier strikes a good balance with regards to complexity and usefulness.

What also looks strange to me is that the linked no_builtin attribute is declared for the callee function, while a CallModifier is currently supplied at the caller's @call.

I don't quite follow here. The Clang attribute is put on a function, and any code or calls within that function won't be transformed to builtins. It's like if you'd manually written every call in the function as @call(.never_intrinsify, ...) in Zig.

@alexrp
Copy link
Member Author

alexrp commented Oct 28, 2024

Updated description to note that this modifier would still permit inlining at the compiler's discretion like auto.

@alexrp alexrp modified the milestones: 0.15.0, unplanned Oct 28, 2024
@dweiller
Copy link
Contributor

dweiller commented Oct 31, 2024

Is this phenomenon intrinsically linked to callsites and/or the function boundary?
I imagine the compiler might replace any code section with an equivalent builtin.
For the use case of implementing compiler_rt, where the functions map 1:1, a call modifier would be a natural fit,
but maybe a nobuiltin/nointrinsify block (similar to nosuspend blocks) would be more flexible / useful in general?

That may be true, but:

1. I don't currently have a motivating use case for such a broad feature.

I don't quite understand how a call modifier for use with @call would help in the linked issue, I may just be misunderstanding that issue. I assumed the linked issue is something like I encountered (detailed below) but I don't know what the zig source for that issue is, so I'm not sure.

One place where being able to mark a function/block with nobuiltin would have been nice to have for me is in implementing things like memcpy in compiler-rt. At the moment, you need to be careful to not have llvm codegen recursive calls to memcpy - basically you have to make sure llvm doesn't recognise code paths (not just function calls) as something it thinks it can replace with a call to memcpy. Sometimes utility functions needed to be made noinline to beat llvm's recognition, and in those cases a nointrinsify call modifier might have been preferable, but I also needed rewrite some simple copying loops as well, as they could be turned into memcpy calls in some cases. Without being able to mark a loop (or the function containing it) with a nobuiltin annotation, this means the code might break whenever llvm changes the way their memcpy detection changes, and can also make some things look more complicated than needed for no apparent reason.

@alexrp
Copy link
Member Author

alexrp commented Oct 31, 2024

It would help because this is the implementation of the problematic function(s):

pub fn __aeabi_memcpy(dest: [*]u8, src: [*]u8, n: usize) callconv(.AAPCS) void {
@setRuntimeSafety(false);
_ = memcpy(dest, src, n);
}

The issue is that LLVM is recognizing the call to memcpy by name and turning it into an intrinsic call instead, which then gets turned into a (recursive) call to __aeabi_memcpy in the target backend.

Putting nobuiltin on the call site will prevent LLVM from doing this.

@alexrp
Copy link
Member Author

alexrp commented Oct 31, 2024

With regards to the compiler-rt problem you're having, I'm afraid you're a victim of this code:

zig/src/codegen/llvm.zig

Lines 3210 to 3217 in a916bc7

if (comp.skip_linker_dependencies or comp.no_builtin) {
// The intent here is for compiler-rt and libc functions to not generate
// infinite recursion. For example, if we are compiling the memcpy function,
// and llvm detects that the body is equivalent to memcpy, it may replace the
// body of memcpy with a call to memcpy, which would then cause a stack
// overflow instead of performing memcpy.
try attributes.addFnAttr(.nobuiltin, &o.builder);
}

The attribute needed to get the desired effect here is no-builtins, not nobuiltin. I mean, obviously!

@ParfenovIgor
Copy link

@alexrp
Copy link
Member Author

alexrp commented Nov 3, 2024

alexrp added a commit to alexrp/zig that referenced this issue Nov 3, 2024
The former prevents recognizing code patterns and turning them into libcalls,
which is what we want for compiler-rt. The latter is meant to be used on call
sites to prevent them from being turned into intrinsics.

Context: ziglang#21833
alexrp added a commit to alexrp/zig that referenced this issue Nov 3, 2024
The former prevents recognizing code patterns and turning them into libcalls,
which is what we want for compiler-rt. The latter is meant to be used on call
sites to prevent them from being turned into intrinsics.

Context: ziglang#21833
@alexrp alexrp linked a pull request Nov 3, 2024 that will close this issue
alexrp added a commit that referenced this issue Nov 4, 2024
The former prevents recognizing code patterns and turning them into libcalls,
which is what we want for compiler-rt. The latter is meant to be used on call
sites to prevent them from being turned into intrinsics.

Context: #21833
alexrp added a commit to alexrp/zig that referenced this issue Nov 5, 2024
…tion.

From `zig build-exe --help`:

  -fno-builtin              Disable implicit builtin knowledge of functions

It seems entirely reasonable and even expected that this option should imply
both no-builtins on functions (which disables transformation of recognized code
patterns to libcalls) and nobuiltin on call sites (which disables transformation
of libcalls to intrinsics). We now match Clang's behavior for -fno-builtin.

In both cases, we're painting with a fairly broad brush by applying this to an
entire module, but it's better than nothing. ziglang#21833 proposes a more fine-grained
way to apply nobuiltin.
alexrp added a commit to alexrp/zig that referenced this issue Nov 5, 2024
…tion.

From `zig build-exe --help`:

  -fno-builtin              Disable implicit builtin knowledge of functions

It seems entirely reasonable and even expected that this option should imply
both no-builtins on functions (which disables transformation of recognized code
patterns to libcalls) and nobuiltin on call sites (which disables transformation
of libcalls to intrinsics). We now match Clang's behavior for -fno-builtin.

In both cases, we're painting with a fairly broad brush by applying this to an
entire module, but it's better than nothing. ziglang#21833 proposes a more fine-grained
way to apply nobuiltin.
alexrp added a commit to alexrp/zig that referenced this issue Dec 5, 2024
alexrp added a commit to alexrp/zig that referenced this issue Dec 7, 2024
alexrp added a commit to alexrp/zig that referenced this issue Jan 24, 2025
alexrp added a commit to alexrp/zig that referenced this issue Jan 24, 2025
@alexrp alexrp closed this as completed in 6ba7855 Feb 24, 2025
emar-kar pushed a commit to emar-kar/zig that referenced this issue Feb 24, 2025
a-khabarov pushed a commit to a-khabarov/zig that referenced this issue Feb 25, 2025
@jacobly0 jacobly0 modified the milestones: unplanned, 0.14.0 Mar 2, 2025
@alexrp alexrp closed this as not planned Won't fix, can't repro, duplicate, stale Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
6 participants