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:
> > > > > 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!
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
