[PATCH] [Refactor] Have getNonLocalPointerDependency take the query instruction

Sanjoy Das sanjoy at playingwithpointers.com
Thu Jan 8 15:40:48 PST 2015


The only thing that concerns me is the new alternate return path on `if (is_ordered(QueryInst)) {`.  With that addressed, LGTM.


================
Comment at: include/llvm/Analysis/MemoryDependenceAnalysis.h:373
@@ -371,2 +372,3 @@
     ///
     /// This method assumes the pointer has a "NonLocal" dependency within BB.
+    void getNonLocalPointerDependency(Instruction *QueryInst,
----------------
What is `BB` here?

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:866
@@ +865,3 @@
+  auto getLocation = [](AliasAnalysis *AA, Instruction *Inst) {
+    if (auto *I = dyn_cast<LoadInst>(Inst))
+      return AA->getLocation(I);
----------------
Minor & optional: I'd just `assert(isa<LoadInst>(I) || isa<StoreInst>(I) ...)` and then `return AA->getLocation(I);` because that makes it obvious that the result is always `AA->getLocation(I)`.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:899
@@ +898,3 @@
+  // TODO: Handle ordered instructions
+  auto is_ordered = [](Instruction *Inst) {
+    if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
----------------
Shouldn't this be `isOrdered`?  Or does llvm allow non camelcase for lambdas?

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:908
@@ +907,3 @@
+
+  if (is_ordered(QueryInst)) {
+    Result.push_back(NonLocalDepResult(FromBB,
----------------
Where did this come from?  Is this a semantic difference?

http://reviews.llvm.org/D6886

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list