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

[serge-sans-paille]

Summary

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 patch harmonizes Clang behavior with GCC and does not generate
initialization code for empty classes under -ftrivial-auto-var-init

[serge-sans-paille]

Comparison with GCC behavior here: https://godbolt.org/z/xc3j9jEsf

[cor3ntin]

clang/lib/CodeGen/CGDecl.cpp

1915–1924

Can we simplify the condition here? this cascade of ternaries is becoming hard to read.

[serge-sans-paille]

switch from cascading ternary operator to cascading if for readability.

[cor3ntin]

clang/lib/CodeGen/CGDecl.cpp

1921–1931

[nickdesaulniers]

clang/lib/CodeGen/CGDecl.cpp

1858

inconsistent east const vs west const

1859–1860

You check if CxxRecordTy is nullptr; do you need to check if Ty is nullptr as well?

1860

What about RecordDecl's in general?

[jfb]

(sorry for sending twice, looks like email reply failed)

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?

Serge provided this example: https://godbolt.org/z/o5asYGq8G

This seems to be "working as designed". Padding can cause leaks, and the design of this feature is to avoid the leaks. Seems like a GCC bug to not fill empty classes.

[serge-sans-paille]

@jfb ok, makes sense. For the record it does show up in our profile but I have been able to workaround it without touching the compiler. This probably mean that the GCC implementation is having a small hole, @jakubjelinek do you think it's worth opening a bug in GCC concerning the lack of memsetting on empty class byte under -ftrivial-auto-var-init?

[serge-sans-paille]

gcc bug filled as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110816

clang/lib/CodeGen/CGDecl.cpp

case LangOptions::TrivialAutoVarInitKind::Pattern: {
     llvm::Value *Done = Builder.CreateICmpEQ(Next, End, "vla-init.isdone");
     Builder.CreateCondBr(Done, ContBB, LoopBB);
     Cur->addIncoming(Next, LoopBB);
     EmitBlock(ContBB);
   } break;
   }
 }
 
+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!");
 
   // If this was emitted as a global constant, we're done.
   if (emission.wasEmittedAsGlobal()) return;
 
   const VarDecl &D = *emission.Variable;
   auto DL = ApplyDebugLocation::CreateDefaultArtificial(*this, D.getLocation());
void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
   bool capturedByInit =
       Init && emission.IsEscapingByRef && isCapturedBy(D, Init);
 
   bool locIsByrefHeader = !capturedByInit;
   const Address Loc =
       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::TrivialAutoVarInitKind::Uninitialized
+                         : getContext()
+                               .getLangOpts()
+                               .getTrivialAutoVarInit())));
 
   auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
     if (trivialAutoVarInit ==
         LangOptions::TrivialAutoVarInitKind::Uninitialized)
       return;
 
     // Only initialize a __block's storage: we always initialize the header.
     if (emission.IsEscapingByRef && !locIsByrefHeader)
       Loc = emitBlockByrefAddress(Loc, &D, /*follow=*/false);
 
     return emitZeroOrPatternForAutoVarInit(type, D, Loc);
   };
 
   if (isTrivialInitializer(Init))
     return initializeWhatIsTechnicallyUninitialized(Loc);

clang/test/CodeGenCXX/auto-var-init-empty-class.cpp

+// 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);
+}

clang/test/CodeGenCXX/auto-var-init.cpp

void test_##NAME##_custom() {                 \
     type_##NAME custom __VA_ARGS__;             \
     used(custom);                               \
   }
 
 // None of the synthesized globals should contain `undef`.
 // 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
 // PATTERN-O1-NOT: @__const.test_small_uninit.uninit
 // PATTERN-O1-NOT: @__const.test_small_custom.custom
 // ZERO-O1-NOT: @__const.test_small_custom.custom
 struct small { char c; };
 // PATTERN-O0: @__const.test_smallinit_uninit.uninit = private unnamed_addr constant %struct.smallinit { i8 [[I8]] }, align 1

 
 TEST_BRACES(bool, bool);
 // CHECK-LABEL: @test_bool_braces()
 // CHECK:       %braces = alloca i8, align [[ALIGN:[0-9]*]]
 // CHECK-NEXT:  store i8 0, ptr %braces, align [[ALIGN]]
 // 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
 // CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
 // PATTERN-LABEL: @test_small_uninit()
 // PATTERN-O0: call void @llvm.memcpy{{.*}} @__const.test_small_uninit.uninit{{.+}}), !annotation [[AUTO_INIT]]
 // PATTERN-O1: store i8 [[I8]], {{.*}} align 1, !annotation [[AUTO_INIT]]
 // ZERO-LABEL: @test_small_uninit()