[PATCH] D112059: Fix inline builtin handling in case of redefinition

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1302
   // in case the function pointer is referenced somewhere.
-  if (FD->isInlineBuiltinDeclaration() && Fn) {
+  if (Fn) {
     std::string FDInlineName = (Fn->getName() + ".inline").str();
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > nickdesaulniers wrote:
> > > > > > I don't think we want to do all this work if just `Fn`; ie. create a new `std::string` with `.inline` suffix for every function we're going to generate code (IR) for.  How about we add an additional unlikely guard:
> > > > > > 
> > > > > > `if (FD->getBuiltinID() && FN) {`
> > > > > > 
> > > > > > Because in the usual case, `FD` both has a builtin ID and is an inline builtin declaration, while in the exceptional case that this patch addresses, `FD` has a builtin ID but is not an inline builtin declaration.
> > > > > Is it correct to gate this on whether it's a builtin or not? I thought that builtin-like (e.g., the usual pile of attributes) user code should also have the same effect, shouldn't it?
> > > > What do you mean? I'm sorry, I don't quite follow.
> > > From the test cases below:
> > > ```
> > > extern inline __attribute__((always_inline)) __attribute__((gnu_inline)) unsigned long strlen(const char *p) {
> > >   return 1;
> > > }
> > > unsigned long mystrlen(char const *s) {
> > >   return strlen(s);
> > > }
> > > unsigned long strlen(const char *s) {
> > >   return 2;
> > > }
> > > ```
> > > These redeclarations resolve a particular way by GCC and this patch is intending to match that behavior. My question ultimately boils down to whether that also happens for this code, where the function is not a builtin but looks like one due to the attributes:
> > > ```
> > > extern inline __attribute__((always_inline)) __attribute__((gnu_inline)) unsigned long wahoo(const char *p) {
> > >   return 1;
> > > }
> > > unsigned long mywahoo(char const *s) {
> > >   return wahoo(s);
> > > }
> > > unsigned long wahoo(const char *s) {
> > >   return 2;
> > > }
> > > ```
> > > If this also reorders, then I don't think we can look at whether `FD->getBuiltinID() != 0` to decide whether to do the reordering dance because arbitrary user functions aren't Clang builtins and so they'd not get the correct behavior. Does that make sense?
> > > If this also reorders
> > 
> > It does; https://godbolt.org/z/bbrox7f6e.
> > 
> > > Does that make sense?
> > 
> > Yes, thanks for the additional info. In this case, I guess we can disregard my feedback that started this thread, marking it as done?
> > 
> > Perhaps @serge-sans-paille should add such a non-builtin test case as part of the change?
> I think you have a valid concern about the extra allocations, but I'm not certain of a better predicate to use. My intuition is that the overhead here won't be unacceptable, but it'd be good to know we're not regressing compile time performance significantly.
> 
> Additional test coverage with a comment as to why we're testing it is a good idea!
Perhaps we can first test whether this FuctionDecl is a redecl, then do the allocation, then check if the `.inline` suffix?

That way we avoid creating the new string unless we're codgen'ing a redecl, which should be uncommon in practice.


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

https://reviews.llvm.org/D112059