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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Thanks for the patch!

Please fix the lint checks.  `git-clang-format HEAD~` should help, and IIRC there is a git hook when using `arc diff` (though maybe that requires one time setup?  I seem to have an `.arclint` file in my `llvm-projects` checkout.



================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1306
+    Fn->setName(Fn->getName() + ".inline");
+  }
+
----------------
avoid `{}` for single statement conditionals.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
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
+//
----------------
Would `-emit-codegen-only` be more appropriate here than `-disable-llvm-passes`?


================
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)) \
----------------
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 at: clang/test/CodeGen/memcpy-inline-builtin.c:20
+void foo(void *a, const void *b, size_t c) {
+  // CHECK: call i8* @memcpy.inline
+  memcpy(a, b, c);
----------------
Can you use `llvm/utils/update_cc_test_checks.py` to autogen full checks for this file? I think it would be helpful to reviewers to be able to see everything that's generated here, even if we only really care that the `call` is to the `.inline` suffixed version.


================
Comment at: clang/test/CodeGen/pr9614.c:44
 // CHECK: declare i8* @memchr(
+// CHECK: declare i32 @abs(i32
 // CHECK: declare void @llvm.prefetch.p0i8(
----------------
Can you elaborate on these changes? Surprising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109967