Update __rust_{alloc,realloc} builtins

[jacob-hughes]

The prototypes for these alloc functions changed in library/alloc and need updating here.

Uncovered as part of zulip discussion here.

[nikic]

The zulip thread mentions that after this change one test no longer optimizes. Is that still the case, or was that with an earlier version?

[jacob-hughes]

Hi @nikic the suite passes fine with these changes. That was user error! When I was working on these changes, I'd inadvertently reordered the builtins, and there is an invariant that these must be defined in alphabetical order -- something I missed as I'd compiled LLVM without assertions 🤦

[nikic]

I've looked into why this works despite the incorrect checks, and it turns out that getAllocationDataForFunction is using the getLibFunc API that only checks the function name, and then performs it's own signature checks. And the parameter counts specified in MemoryBuiltins were correct. As such, the libcall detection logic ultimately didn't matter.

Possibly it makes sense to just drop these checks entirely, if they're going to be ignored anyway? Though we could also add some additional FuncAttrs inference, in particular adding noalias, inaccessible_mem_only and willreturn attributes may be beneficial (and that part would require passing signature checks).

[jacob-hughes]

That's interesting, and explains a lot. I'm happy to remove these checks entirely if you'd prefer.

Though we could also add some additional FuncAttrs inference, in particular adding noalias, inaccessible_mem_only and willreturn attributes may be beneficial (and that part would require passing signature checks).

IIUC noalias is already passed to the callsite with the #[rustc_allocator] attribute on __rustc_alloc. What benefit is gained by adding it here also?

[nikic]

Though we could also add some additional FuncAttrs inference, in particular adding noalias, inaccessible_mem_only and willreturn attributes may be beneficial (and that part would require passing signature checks).

IIUC noalias is already passed to the callsite with the #[rustc_allocator] attribute on __rustc_alloc. What benefit is gained by adding it here also?

Right. For reference, this happens here:

https://github.com/rust-lang/rust/blob/38030ffb4e735b26260848b744c0910a5641e1db/compiler/rustc_codegen_llvm/src/attributes.rs#L302-L304

I guess if we wanted to add more attributes, it would make sense to do so there as well, no point in doing it on the LLVM side really.

I think in that case it would be best to just drop this code.

[jacob-hughes]

Do you mean close the PR? Or remove the rust alloc functions from the TargetLibraryInfo parts of LLVM?

[nikic]

Do you mean close the PR? Or remove the rust alloc functions from the TargetLibraryInfo parts of LLVM?

I mean remove the signature checks in TargetLibraryInfo.cpp handling, as they are effectively unused.

[jacob-hughes]

Something like this 40d6804?

[nikic]

Due to changes in LLVM 13 these checks actually matter now, so I've included the original version of this PR as part of the rebase (b1f55f7 is the new commit).