[PATCH] D109749: Experimental Partial Mem2Reg

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Nice! I like this, but now i have a fundamental concern.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4709-4710
+
+  for (BasicBlock::iterator I = EntryBB.begin(), E = std::prev(EntryBB.end());
+       I != E; ++I) {
+    AllocaInst *AI = dyn_cast<AllocaInst>(I);
----------------



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4715-4720
+    // We're only interested if the alloca is used by a non-intrinsic
+    // call instruction...
+    if (!any_of(AI->users(), [](const User *U) {
+          return isa<CallInst>(U) && !isa<IntrinsicInst>(U);
+        }))
+      continue;
----------------
This isn't right. What if not the `alloca`, but `gep(alloca)`, is passed into the function?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4736-4739
+    for (User *U : AI->users()) {
+      CallInst *CI = dyn_cast<CallInst>(U);
+      if (!CI)
+        continue;
----------------
Same, this should recurse down the uses of an alloca.


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:105-106
+      bool NoCaptures = true;
+      for (unsigned i = 0; i < CI->arg_size(); ++i)
+        if (CI->getArgOperand(i) == AI)
+          NoCaptures &= CI->paramHasAttr(i, Attribute::NoCapture);
----------------



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

https://reviews.llvm.org/D109749