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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
