[PATCH] D34311: [InstCombine] Don't replace allocas with globals if we can't prove that it's large enough.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Hmm, actually, thinking about it a bit more, I'm not sure this patch is right.

If you were to assume that out-of-bounds loads return undef rather than have undefined behavior, this patch would be unnecessary.  There's only one memcpy that writes to the alloca.  That memcpy writes N known bytes to the alloca.  Any other instruction which accesses the alloca is either reading one of those N bytes, or returns undef, so the returned values are consistent with reading directly from the global.

Of course, that isn't the world we live in; we can't speculate out-of-bounds loads.  Therefore, the safety check needs to make sure we aren't introducing any such loads.  There are a couple of ways to do that:

1. Prove that the constant pointer points to at least `sizeof(alloca type)` bytes of memory.  The loads in the rewritten code are out-of-bounds only if they were out-of-bounds in the original code.
2. Ensure that every load is dominated by the initializing memcpy, and the memcpy copies at least `sizeof(alloca type)` bytes of memory.  If the memcpy is copying too many bytes, it's immediately undefined behavior.  Any load dominated by the memcpy is one of the following: in-bounds in the original code, out-of-bounds in the original code, or dominated by undefined behavior.

This patch implements (2)... but only partially: it's missing the dominance check.  Testcase something like this:

  const char x[1] = { 3 };
  char z[10];
  int f(bool this_is_false) {
    char y[10];
    if (this_is_false) memcpy(y, x, 10); 
    memcpy(z, y, 10); 
  }



================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:175
+
+  uint64_t CopySize = CopyLength->getZExtValue();
+  assert(CopySize <= AllocaSize);
----------------
getZExtValue() will crash on very large lengths.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:176
+  uint64_t CopySize = CopyLength->getZExtValue();
+  assert(CopySize <= AllocaSize);
+  if (CopySize < AllocaSize)
----------------
This assertion doesn't seem justified; sure, the memcpy has undefined behavior, but that shouldn't crash the compiler.


https://reviews.llvm.org/D34311