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