[PATCH] D159075: [MemCpyOpt] implement forward dataflow sensitive stack-move optimization

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Just some style notes, I haven't tried to understand the algorithm yet.



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1427
+  };
+};
+using InstModRefMap =
----------------
This should be in an anonymous namespace.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1435
+// if it's modified by others before Def.
+bool IsDef(Instruction *UI, std::optional<TypeSize> AllocaSize,
+           const DataLayout &DL) {
----------------
Should be static.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1435
+// if it's modified by others before Def.
+bool IsDef(Instruction *UI, std::optional<TypeSize> AllocaSize,
+           const DataLayout &DL) {
----------------
nikic wrote:
> Should be static.
We shouldn't need std::optional here, at this point we know the alloca is sized.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1456
+        Size < 0 || uint64_t(Size) == AllocaSize)
+      return true;
+  } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(II)) {
----------------
Does this actually do anything? It seems like the lifetime handling in CheckModRefConflict will skip this code path already.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1483
+  return L;
+}
+inline bool isSrcMod(const ModState MS) {
----------------
Could use BitmaskEnum.h here instead?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1503
+// users) complexity.
+bool CheckModRefConflict(BasicBlockInstModRefMap BBInstModRefMap,
+                         AllocaInst *AI, Instruction *Store,
----------------
static

Is passing the map by value here intentional?


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

https://reviews.llvm.org/D159075