[PATCH] D90104: [LoopUnroll] Duplicate noalias metadata

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this makes sense, though it will probably only mitigate the problem.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:262
+    const MDNode *M = cast<MDNode>(Queue.pop_back_val());
+    for (unsigned i = 0, ie = M->getNumOperands(); i != ie; ++i)
+      if (const MDNode *M1 = dyn_cast<MDNode>(M->getOperand(i)))
----------------
Nit: Don't we have a range based version of this?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:273
+
+static AliasMetadataMap cloneAliasMetadata(const SetVector<const MDNode *> MD) {
+  SmallVector<TempMDTuple, 16> DummyNodes;
----------------
Do you pass MD by value on purpose? Also consider passing the return value by reference, though I doubt it makes much of a difference.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:285
+  for (const MDNode *I : MD) {
+    SmallVector<Metadata *, 4> NewOps;
+    for (unsigned i = 0, ie = I->getNumOperands(); i != ie; ++i) {
----------------
Nit: hoist and use clear, according to the programmers manual (or some other document we have).


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:286
+    SmallVector<Metadata *, 4> NewOps;
+    for (unsigned i = 0, ie = I->getNumOperands(); i != ie; ++i) {
+      const Metadata *V = I->getOperand(i);
----------------
Nit: range again?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:304
+
+static void remapAliasMetadata(AliasMetadataMap &MDMap,
+                               ValueToValueMapTy &VMap) {
----------------
Could you add some documentation here and above please.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:307
+  for (ValueToValueMapTy::iterator VMI = VMap.begin(), VMIE = VMap.end();
+       VMI != VMIE; ++VMI) {
+    if (!VMI->second)
----------------
Nit: range & auto?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90104