[PATCH] D137707: Move "auto-init" instructions to the dominator of their users

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

High level notes:

- I'm not sure I fully understand the motivation for using MemorySSA here. Unless I'm missing something, It seems like when it comes to optimizing allocas, it would be sufficient to look at non-transparent users of the alloca (by which I mean, in first approximation, look through bitcast, gep, count everything else). The review mentions fences, but wouldn't these be covered as long as you consider pointer captures as "uses"?
- This is missing some tests where there are other memory instructions beyond the auto-init stores. You should find that, as implemented, these are going to block the transform even if they are "obviously" unrelated. If they are MemoryUses, you can recover this by requesting optimized use form. However, MemoryDefs always depend on the preceding MemoryDef, even if they are non-clobbering. This means that your optimization will always stop at the next MemoryDef, even if it is to a different alloca. This is a big limitation. You could do alias checks to skip those, but that's also the point where this becomes an expensive transform.
- There is a pending patch to add sinking support to DSE (https://reviews.llvm.org/D136218). I haven't really looked at it yet, but I expect it does the MSSA-based transform in the right way (i.e. skipping non-clobbering instructions). This brings me back to the first point...



================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:36
+
+bool hasAutoInitMetadata(Instruction const &I) {
+  return I.hasMetadata(LLVMContext::MD_annotation) &&
----------------
Use `static` instead of anon namespace (see https://llvm.org/docs/CodingStandards.html#anonymous-namespaces).

Also please prefer the more typical `const Instruction &I` spelling.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:66
+      Instruction *MI = M->getMemoryInst();
+      if (MI->isLifetimeStartOrEnd() || MI == I)
+        continue;
----------------
The lifetime intrinsic check is missing test coverage.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:75
+      WorkList.append(UsersAsMemoryAccesses.begin(),
+                      UsersAsMemoryAccesses.end());
+    }
----------------
`append_range(WorkList, UsersAsMemoryAccesses)`


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:93
+    // The remaining logic should be pretty similar.
+    if (!hasAutoInitMetadata(I))
+      continue;
----------------
Assert or check that use_empty(), the transform relies on it.

I'd also assert or check that the instruction is not volatile. (Not sure if auto-init can be volatile...)


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:155
+    // execution, and guarded by at least one condition.
+    JobList.emplace_back(&I, &*UsersDominator->getFirstInsertionPt());
+  }
----------------
Is it possible for this to be a catchswitch block?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:164
+
+  for (auto &Job : JobList) {
+    Job.first->moveBefore(Job.second);
----------------
Drop braces.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:168
+
+  NumMoved += JobList.size();
+
----------------
`verifyMemorySSA()` here -- pretty sure there will be verification errors. E.g. you might be moving past unrelated MemoryInsts.


================
Comment at: llvm/test/Transforms/MoveAutoInit/branch.ll:4
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
----------------
Shouldn't be relevant.


================
Comment at: llvm/test/Transforms/MoveAutoInit/branch.ll:30
+if.then:                                          ; preds = %entry
+  %arrayidx = getelementptr inbounds [8 x i32], ptr %buffer, i64 0, i64 0
+  call void @dump(ptr %arrayidx)
----------------
Zero-index GEP is redundant.


================
Comment at: llvm/test/Transforms/MoveAutoInit/fence.ll:24
+  %tobool = icmp ne i32 %x, 0
+  fence acquire
+  br i1 %tobool, label %if.then, label %if.end
----------------
To make this test more meaningful, you probably want to capture `%val` before the fence? Otherwise the initialization is unobservable.


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

https://reviews.llvm.org/D137707