[PATCH] D109967: Simplify handling of builtin with inline redefinition

[serge via Phabricator via cfe-commits]

serge-sans-paille marked 6 inline comments as done.
serge-sans-paille added inline comments.


================
Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s
+//
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Would `-emit-codegen-only` be more appropriate here than `-disable-llvm-passes`?
> No; but I don't think we need `-disable-llvm-passes` since we haven't explicitly enabled additional optimization modes via `-O` flags? ie. I suspect if we drop `-disable-llvm-passes` then nothing changes?
`-disable-llvm-passes` was tehre to disable the always inliner. I used another approach to keep merker from the inlined function and keep the test easy to review.


================
Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:7
+
+// always_inline is used so clang will emit this body. Otherwise, we need >= -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
----------------
nickdesaulniers wrote:
> I'm not sure what `this` refers to in the comment?
> 
> Also, this whole bit about `always_inline` smells fishy to me.  The semantics of `gnu_inline` is that no body is emitted.  If you //don't// want a body //don't// use `gnu_inline`.  Or was this set of qualifiers+fn attrs taken directly from the kernel, or glibc?  `FunctionDecl::isInlineBuiltinDeclaration` only checks for builtin ID, has body, and whether inline was specified.
Comment updated. Basically these attributes are needed for a function to be considered by clang as redfinable, which is the case here. these attributes are extensively used in glibc headers for that purpose.


================
Comment at: clang/test/CodeGen/pr9614.c:44
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
----------------
nickdesaulniers wrote:
> Can you elaborate on these changes? Surprising.
Before the patch, the redefined version of bas was ignored as the function is a trivially recursive redefinition. Thanks to current patch, the redefinition is correctly detected, renamed and inlined (the always inliner still runs here)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109967/new/

https://reviews.llvm.org/D109967