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:
> > > > > > 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.
Ah, ok then.
> Additional test coverage with a comment as to why we're testing it is a good
> idea!
Yeah, with that and fixes to the other small nits in the test case, then I
think this is ready to land.
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
