[PATCH] D155580: [trivial-auto-var-init] Do not emit initialization code for empty class

[JF Bastien via cfe-commits]

On Tue, Jul 18, 2023 at 8:41 PM serge via Phabricator <
reviews at reviews.llvm.org> wrote:

> serge-sans-paille created this revision.
> serge-sans-paille added reviewers: nickdesaulniers, jfb, aaron.ballman.
> Herald added a subscriber: kristof.beyls.
> Herald added a project: All.
> serge-sans-paille requested review of this revision.
> Herald added a project: clang.
> Herald added a subscriber: cfe-commits.
>
> Empty class still use one byte, but there is no way to read that byte
> programmatically in a legitimate way. Yet trivial auto var init always
> generate a store for it and there is no programmatic way to prevent the
> generation of that extra store.



This is the same as padding, and is initialized on purpose. If it’s truly
unused then the optimizer will eliminate it… unless it can’t prove whether
the store is dead.

Does this show up in any meaningless performance case? If so, can we
optimize it away?



This patch harmonizes Clang behavior with GCC and does not generate
> initialization code for empty classes under -ftrivial-auto-var-init
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> https://reviews.llvm.org/D155580
>
> Files:
>   clang/lib/CodeGen/CGDecl.cpp
>   clang/test/CodeGenCXX/auto-var-init-empty-class.cpp
>   clang/test/CodeGenCXX/auto-var-init.cpp
>
>
> Index: clang/test/CodeGenCXX/auto-var-init.cpp
> ===================================================================
> --- clang/test/CodeGenCXX/auto-var-init.cpp
> +++ clang/test/CodeGenCXX/auto-var-init.cpp
> @@ -43,9 +43,6 @@
>  // PATTERN-NOT: undef
>  // ZERO-NOT: undef
>
> -// PATTERN-O0: @__const.test_empty_uninit.uninit = private unnamed_addr
> constant %struct.empty { i8 [[I8]] }, align 1
> -// PATTERN-O1-NOT: @__const.test_empty_uninit.uninit
> -struct empty {};
>  // PATTERN-O0: @__const.test_small_uninit.uninit = private unnamed_addr
> constant %struct.small { i8 [[I8]] }, align 1
>  // PATTERN-O0: @__const.test_small_custom.custom = private unnamed_addr
> constant %struct.small { i8 42 }, align 1
>  // ZERO-O0: @__const.test_small_custom.custom = private unnamed_addr
> constant %struct.small { i8 42 }, align 1
> @@ -586,24 +583,6 @@
>  // CHECK-NOT:   !annotation
>  // CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
>
> -TEST_UNINIT(empty, empty);
> -// CHECK-LABEL: @test_empty_uninit()
> -// CHECK:       %uninit = alloca %struct.empty, align
> -// CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
> -// PATTERN-LABEL: @test_empty_uninit()
> -// PATTERN-O0: call void @llvm.memcpy{{.*}} @__const.test_empty_uninit.uninit{{.+}}),
> !annotation [[AUTO_INIT]]
> -// PATTERN-O1: store i8 [[I8]], {{.*}} align 1, !annotation [[AUTO_INIT]]
> -// ZERO-LABEL: @test_empty_uninit()
> -// ZERO-O0: call void @llvm.memset{{.*}}, i8 0,{{.+}}), !annotation
> [[AUTO_INIT]]
> -// ZERO-O1: store i8 0, {{.*}} align 1, !annotation [[AUTO_INIT]]
> -
> -TEST_BRACES(empty, empty);
> -// CHECK-LABEL: @test_empty_braces()
> -// CHECK:       %braces = alloca %struct.empty, align
> -// CHECK-NEXT:  call void @llvm.memcpy
> -// CHECK-NOT:   !annotation
> -// CHECK-NEXT:  call void @{{.*}}used{{.*}}%braces)
> -
>  TEST_UNINIT(small, small);
>  // CHECK-LABEL: @test_small_uninit()
>  // CHECK:       %uninit = alloca %struct.small, align
> Index: clang/test/CodeGenCXX/auto-var-init-empty-class.cpp
> ===================================================================
> --- /dev/null
> +++ clang/test/CodeGenCXX/auto-var-init-empty-class.cpp
> @@ -0,0 +1,18 @@
> +// RUN: %clang_cc1 -triple x86_64-unknown-unknown
> -ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s
> +// RUN: %clang_cc1 -triple x86_64-unknown-unknown
> -ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
> +
> +// C++ empty classes still take some memory, but there is no need to
> initialize
> +// it has it cannot be accessed. This matches GCC behavior.
> +
> +struct S {
> +};
> +
> +extern "C" void use(struct S*);
> +
> +// CHECK-LABEL: @empty_class(
> +// CHECK:       %s = alloca
> +// CHECK-NEXT:  call void @use(ptr noundef %s)
> +extern "C" void empty_class(void) {
> +  S s;
> +  return use(&s);
> +}
> Index: clang/lib/CodeGen/CGDecl.cpp
> ===================================================================
> --- clang/lib/CodeGen/CGDecl.cpp
> +++ clang/lib/CodeGen/CGDecl.cpp
> @@ -1855,6 +1855,13 @@
>    }
>  }
>
> +static bool isEmptyClass(VarDecl const &D) {
> +  const Type *Ty = D.getType().getTypePtr();
> +  if (const auto *CxxRecordTy = Ty->getAsCXXRecordDecl())
> +    return CxxRecordTy->isEmpty();
> +  return false;
> +}
> +
>  void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
>    assert(emission.Variable && "emission was not valid!");
>
> @@ -1903,12 +1910,18 @@
>        locIsByrefHeader ? emission.getObjectAddress(*this) :
> emission.Addr;
>
>    // Note: constexpr already initializes everything correctly.
> +  // Note: empty classes still use a byte, but it's ok not to initialize
> it as
> +  // it cannot be legitimately accessed.
>    LangOptions::TrivialAutoVarInitKind trivialAutoVarInit =
>        (D.isConstexpr()
>             ? LangOptions::TrivialAutoVarInitKind::Uninitialized
>             : (D.getAttr<UninitializedAttr>()
>                    ? LangOptions::TrivialAutoVarInitKind::Uninitialized
> -                  : getContext().getLangOpts().getTrivialAutoVarInit()));
> +                  : (isEmptyClass(D)
> +                         ? LangOptions::TrivialAutoVarIni
> tKind::Uninitialized
> +                         : getContext()
> +                               .getLangOpts()
> +                               .getTrivialAutoVarInit())));
>
>    auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
>      if (trivialAutoVarInit ==
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230718/66e27cab/attachment-0001.html>