aaron.ballman 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();
----------------
nickdesaulniers wrote:
> 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.
That could save us a bit of perf, but I think redecls are actually quite common
because a definition is itself a declaration, so having a decl in a header file
and defn in a source file will produce a redecl chain.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112059/new/
https://reviews.llvm.org/D112059
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
