[PATCH] D131628: [LangRef] Add description for nocallback attribute

Sat Aug 13 08:57:54 PDT 2022

nlopes added a comment.

In D131628#3719212 <https://reviews.llvm.org/D131628#3719212>, @jdoerfert wrote:

> In D131628#3719136 <https://reviews.llvm.org/D131628#3719136>, @nlopes wrote:
>
>> Anyway, the semantics issue pointed out above must be resolved or else the patch must be reverted as currently the lowering of clang is wrong and will miscompile programs.
>
> Let's just resolve this now and here.
>
> I figured us using it implies we need it. However, here is some data: https://tianshilei.me/wp-content/uploads/2022/06/ipdps-2022.pdf

My question is normal and reasonable. I ask it every time someone wants to add something new to the IR.
The LLVM community must know why it has to deal with extra complexity forever. I will refuse any IR feature that gives 0.00001% speedup for 1 benchmark.

> Note section IV-B2 which talks about inter-procedural reachability, that is only possible with nocallback on intrinsics (and leaf on malloc and free in some cases).

No numbers are provided. My question still stands.
Plus, malloc/free have the `inaccessiblememonly` attribute, which means they don't need the `nocallback` attribute. These attributes are transitive, so it's a given that malloc cannot call any function that doesn't have the `inaccessiblememonly` attribute.
What's the impact of this attribute in your reachability analysis?

>> 2. [...]  A simple hack would be to drop all nocallback attributes when linking IR files. That would fix the LTO case.
>
> `nocallback` follows, as I mentioned before, the same idea as `inaccesiblememory`. You are not wrong about LTO, but we can apply the same handling to both which is to drop the attribute during a llvm-link step. We only need to do that if we link in the definition. That is why I said this should only be valid on declarations. (inaccessiblememory should also have that restriction).

It's not the same thing as `nocallback` is given by users and `inaccessiblememonly` is not. LLVM adds the latter only to libcalls. It's not always wrong to do LTO with it; only if a lib function calls another function and they share the same globals -- not sure it exists in practice.
Nevertheless, we cannot introduce a new IR feature that we know that is buggy and without a plan to fix it. Absolutely no way! The patch must be reverted first. Then you can propose a plan to reintroduce the feature in a sound way if it's justified. We need numbers please.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131628