[PATCH] D112059: Fix inline builtin handling in case of redefinition

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6bfc85c217e4: Fix inline builtin handling in case of redefinition (authored by serge-sans-paille). Changed prior to commit:

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers accepted this revision. nickdesaulniers added a comment. thanks again for all of the work that went into this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 ___ cfe-commits mailing

[PATCH] D112059: Fix inline builtin handling in case of redefinition

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a formatting nit. I don't think the precommit CI failures are related to your patch from what I was seeing, but may be worth keeping an eye on once you land just in case. Comment at:

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 383241. serge-sans-paille added a comment. Re-uploading previous version that walks redef, with a slight change in the walking algorithm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 Files:

[PATCH] D112059: Fix inline builtin handling in case of redefinition

aaron.ballman added a comment. In D112059#3094464 , @nickdesaulniers wrote: > Here's a [hastily and poorly written] script to measure the average cycle > counts for 30 invocations using linux `perf`: >

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added a comment. Here's a [hastily and poorly written] script to measure the average cycle counts for 30 invocations using linux `perf`: https://gist.github.com/nickdesaulniers/4a20ba10c26ac2ad02cb0425b8b0f826 For Diff 382671 (latest; storing redecl state), builds of the linux

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille added a comment. I second @aaron.ballman there. I compiled the sqlite3.c amalgamation, -O0, with both approach, measuring the number of instructions as gathered by `valgrind --tool=callgrind` - when walking redecls:9001630039 instructions, I changed the

[PATCH] D112059: Fix inline builtin handling in case of redefinition

aaron.ballman added a comment. In D112059#3090466 , @serge-sans-paille wrote: > - Use a FunctionDecl Attribute to store the shadowed inline redecl status The downside to this approach is that we can now handle less ctor initializers because we need to

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille added inline comments. Comment at: clang/test/CodeGen/user-func-gnu-inline-redecl.c:20 + return some_size(s); +} nickdesaulniers wrote: > this test passes before this patch is applied; I wonder if we have existing > coverage in tree for this

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 382671. serge-sans-paille added a comment. - Use a FunctionDecl Attribute to store the shadowed inline redecl status CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 Files:

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added inline comments. Comment at: clang/test/CodeGen/user-func-gnu-inline-redecl.c:20 + return some_size(s); +} this test passes before this patch is applied; I wonder if we have existing coverage in tree for this case? Surprisingly, I don't

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added a comment. Actually, it looks like: diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 69d2ef631872..8e77cdef2ed5 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -10927,6 +10927,10 @@ bool

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers accepted this revision. nickdesaulniers added a comment. Thank you for fixing this terrible edge case; LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 ___ cfe-commits mailing

[PATCH] D112059: Fix inline builtin handling in case of redefinition

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, though codegen is not my area of expertise. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 382344. serge-sans-paille added a comment. Formatting nits CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 Files: clang/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 382337. serge-sans-paille added a comment. - Add a test case to ensure we keep the right behavior for non-intrinsic gnu inline - walk the redecl chain before doing an extra string alloc CHANGES SINCE LAST ACTION

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added a comment. Bumping for an update here. We can tolerate a build breakage for our older kernels over the weekend, but we should really try to get this resolved by EOW, otherwise we need to look into reverting: - 3d6f49a56995b845c40be5827ded5d1e3f692cec

[PATCH] D112059: Fix inline builtin handling in case of redefinition

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() +

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added subscribers: aaron.ballman, rsmith. 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) {

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 381014. serge-sans-paille added a comment. Avoid walking redecls CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 Files: clang/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille updated this revision to Diff 381011. serge-sans-paille added a comment. Reduce the number of time we would walk redecls. Simplify test case CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 Files:

[PATCH] D112059: Fix inline builtin handling in case of redefinition

nickdesaulniers added a subscriber: nathanchance. nickdesaulniers added a comment. Yes; GCC does behave this way. It does not consider a non-gnu-inline redefinition an error, and it does seem to prefer the non-gnu-inline redeclaration when both are present, AFAICT. The test is verifying that

[PATCH] D112059: Fix inline builtin handling in case of redefinition

manojgupta added a comment. thanks, I can verify that it fixes the crash we were seeing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112059/new/ https://reviews.llvm.org/D112059 ___ cfe-commits

[PATCH] D112059: Fix inline builtin handling in case of redefinition

serge-sans-paille created this revision. serge-sans-paille added reviewers: nickdesaulniers, manojgupta. serge-sans-paille requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Basically, inline builtin definition are shadowed by externally