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