[PATCH] D97735: [Globals] Treat nobuiltin fns as maybe-derefined.

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:147
+      // derefined.
+      return isInterposable() || isNobuiltinFnDef();
     }
----------------
jeroen.dobbelaere wrote:
> fhahn wrote:
> > jdoerfert wrote:
> > > rsmith wrote:
> > > > I don't think `nobuiltin` is relevant; I think what matters is whether the function is a builtin function. We have the same miscompile if we removed all the `builtin` / `nobuiltin` annotations from the IR generated for the PR49022 example, and this change does nothing to address that case.
> > > So you are saying we can legally define our own `operator delete(void*) = _ZdlPv` without a `nobuiltin` attribute and that would still need to stop IPO from assuming it is the idealized version we all know and love?
> > I think the problem is how would we detect that a given global value may be used with builtin semantics? 
> > 
> > Do frontends create function definitions for recognized builtins without `nobuiltin`? If frontends need to do so, can they add an attribute to indicate that this functions may be used with some builtin semantics?
> > I think the problem is how would we detect that a given global value may be used with builtin semantics? 
> > 
> > Do frontends create function definitions for recognized builtins without `nobuiltin`? If frontends need to do so, can they add an attribute to indicate that this functions may be used with some builtin semantics?
> 
> How can frontends know about 'future builtins' ? Should they already annotate all functions as 'nobuiltin' (except for the known builtins).
> What about the following:   https://www.godbolt.org/z/To69x5
> A 'c' program where we happen to use the encoded 'new/delete' names. I don't think llvm should optimize those.
> 
> 
> 
I have the same thoughts as @jdoerfert. If there is no `nobuiltin` annotation for the builtin definition, the optimization to trap for the PR49022 example should be expected and not a bug. 

I agree with @rsmith 's that it is whether the function is a builtin function matter, not the annotations. However, it seems that `builtin` annotation is not checked anywhere and only emitted for C++ global replacement new/delete, so basically it is a no-op for LLVM. `nobuiltin` annotation is emitted for all builtin definition with `inline` specifier (https://github.com/llvm/llvm-project/blob/d56729b4a4393da9c65bdfe762b51f8b7b0ce0ca/clang/lib/CodeGen/CodeGenModule.cpp#L2097). So `nobuiltin` is almost the same as saying this is a builtin definition. To achieve what we want, just make sure `nobuiltin` annotation is also emitted for any builtin definition without `inline` specifier. This inhibits IPO for all builtin definitions, how much performance would be lost due to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97735