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

[wael yehia via Phabricator via llvm-commits]

w2yehia added inline comments.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:147
+      // derefined.
+      return isInterposable() || isNobuiltinFnDef();
     }
----------------
ychen wrote:
> 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?
just wanna point out that _ZdlPv and _Znwm fall under the reserved names, and so the compiler might be allowed to assume certain stuff about such symbols.
> C11 standard (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf)
> 7.1.3 Reserved Identifiers:
> All identifiers that begin with an underscore and either an uppercase letter or another
underscore are always reserved for any use



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97735