[PATCH] isDereferenceablePointer: look through gc.relocates + test

Philip Reames listmail at philipreames.com
Fri Jan 23 11:16:46 PST 2015


I'd suggest splitting this into two patches: adding the new analysis, adding the new optimization and testing it.  If you really didn't want to for some reason, it's not that big a deal, but it would make it easier to review quickly.


================
Comment at: include/llvm/IR/InstIterator.h:130
@@ -129,1 +129,3 @@
 inline inst_iterator inst_end(Function *F)   { return inst_iterator(*F, true); }
+inline iterator_range<inst_iterator> insts(Function *F) {
+  return iterator_range<inst_iterator>(inst_begin(F), inst_end(F));
----------------
I might call this inst_range.  I'm usually not a fan of extra verbosity, but I see potential confusion resulting from a such short relatively common name being added to the llvm namespace.  

================
Comment at: lib/Analysis/MemDepPrinter.cpp:147
@@ -146,6 +146,3 @@
 
-    for (DepSet::const_iterator I = InstDeps.begin(), E = InstDeps.end();
-         I != E; ++I) {
-      const Instruction *DepInst = I->first.getPointer();
-      DepType type = I->first.getInt();
-      const BasicBlock *DepBB = I->second;
+    for (auto I: InstDeps) {
+      const Instruction *DepInst = I.first.getPointer();
----------------
Please separate this refactoring into a separate change.

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:1
@@ +1,2 @@
+//===- MemDerefPrinter.cpp - Printer for isDereferenceablePointer ---------===//
+//
----------------
Please split the introduction of the printer and one or two example tests into it's own change.  I'll be happy to LGTM that one quickly.  

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:22
@@ +21,3 @@
+  struct MemDerefPrinter : public FunctionPass {
+    const Function *F;
+
----------------
Why do you need to capture F?

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:24
@@ +23,3 @@
+
+    enum DepType {
+      Clobber = 0,
----------------
Why is this enum needed?

================
Comment at: lib/Analysis/MemDerefPrinter.cpp:43
@@ +42,3 @@
+    void releaseMemory() override {
+      F = nullptr;
+    }
----------------
You should also clear your small vector.  (I think)

================
Comment at: lib/IR/Value.cpp:575
@@ -572,1 +574,3 @@
 
+  // For gc.relocate, look through relocations
+  if (const IntrinsicInst *I = dyn_cast<IntrinsicInst>(V))
----------------
I'd suggest using isGCRelocate here.

http://reviews.llvm.org/D7075

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






More information about the llvm-commits mailing list