[PATCH] D54604: Automatic variable initialization

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: include/clang/Driver/ToolChain.h:355
+  virtual LangOptions::TrivialAutoVarInitKind
+  GetDefaultTrivialAutoVarInit() const {
+    return LangOptions::TrivialAutoVarInitKind::Uninitialized;
----------------
I wouldn't introduce this function because nothing is overriding it (for now, at least).


================
Comment at: lib/CodeGen/CGDecl.cpp:968
+  // The following value is a guaranteed unmappable pointer value and has a
+  // reperated byte-pattern which makes it easier to synthesize. We use it for
+  // pointers as well as integers so that aggregates are likely to be
----------------
*repeated


================
Comment at: lib/CodeGen/CGDecl.cpp:977
+  constexpr uint32_t SmallValue = 0x000000AA;
+  if (Ty->isIntOrIntVectorTy()) {
+    unsigned BitWidth = llvm::cast<llvm::IntegerType>(
----------------
Do you have test coverage for vectors?


================
Comment at: lib/CodeGen/CGDecl.cpp:978
+  if (Ty->isIntOrIntVectorTy()) {
+    unsigned BitWidth = llvm::cast<llvm::IntegerType>(
+                            Ty->isVectorTy() ? Ty->getVectorElementType() : Ty)
----------------
Is it necessary to qualify `cast` with `llvm::`? It doesn't seem to be qualified elsewhere in this file. (Same below.)


================
Comment at: lib/CodeGen/CGDecl.cpp:1145
+  if (Ty->isStructTy() || Ty->isArrayTy() || Ty->isVectorTy()) {
+    for (unsigned Op = 0, NumOp = constant->getNumOperands(); Op != NumOp;
+         ++Op) {
----------------
Maybe use `User::operands()` to enumerate operands here?


================
Comment at: lib/CodeGen/CGDecl.cpp:1631
+  auto initializeWhatIsTechnicallyUninitialized = [&]() {
+    if (trivialAutoVarInit !=
+        LangOptions::TrivialAutoVarInitKind::Uninitialized) {
----------------
Is it possible to rewrite this function using early returns to avoid all the indentation below for the VLA case? I'd also put the code for non-VLAs first to make this function easier to understand (on my first reading I somehow got the impression that this function is *only* handling VLAs). I'm thinking maybe something like:

```
if (trivialAutoVarInit == LangOptions::TrivialAutoVarInitKind::Uninitialized)
  return;

if (!getContext().getTypeSizeInChars(type).isZero()) {
  // handle non-VLAs
}

auto *VlaType = dyn_cast_or_null<VariableArrayType>(getContext().getAsArrayType(type));
if (!VlaType)
  return;

// handle VLAs
```


================
Comment at: lib/CodeGen/CGDecl.cpp:1673
+            llvm::BasicBlock *ContBB = createBasicBlock("vla-init.cont");
+            EmitBlock(LoopBB);
+            llvm::PHINode *Cur =
----------------
I think we need to check that the size is non-zero here. Otherwise we're going to clobber whatever comes after the VLA if its size is zero (I know that C forbids zero-sized VLAs, but I believe we support them as a GNU extension).


================
Comment at: test/CodeGenCXX/trivial-auto-var-init.cpp:5
+
+// PATTERN-NOT: undef
+// ZERO-NOT: undef
----------------
Since this is your only test involving structs and arrays, you probably want to test them beyond the fact that they don't contain undef. I also believe that this is testing for the absence of `undef` before the first label and not the total absence of `undef` in the output.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54604