[PATCH] [OPENMP] A helper for marking intstructions with llvm.mem.parallel_loop_access

hfinkel at anl.gov hfinkel at anl.gov
Wed May 21 09:10:47 PDT 2014


With these changes, LGTM.

================
Comment at: lib/CodeGen/CGBuilder.h:50
@@ -23,2 +49,3 @@
+  CGBuilderTy;
 #endif
 
----------------
This is now a lot of parameters to be duplicated in both sizes of the NDEBUG conditional. You can, for example, define a boolean that depends on NDEBUG and use that instead.

================
Comment at: lib/CodeGen/CGLoopInfo.cpp:89
@@ +88,3 @@
+void LoopInfoStack::pop() {
+  assert(!Active.empty());
+  Active.pop_back();
----------------
This assert is not self explanatory. Please write:
  assert(!Active.empty() && "No active loops to pop");
(or something like that).

================
Comment at: lib/CodeGen/CGLoopInfo.cpp:115
@@ +114,3 @@
+      LI->setMetadata("llvm.mem.parallel_loop_access", L.getLoopID());
+    }
+  }
----------------
This is not quite complete. It misses memory intrinsics, for example. The check in LLVM's lib/Analysis/LoopInfo.cpp requires metadata on all instructions for which mayReadOrWriteMemory() returns true.

I think this can be:

  if (L.getAttributes().IsParallel && I->mayReadOrWriteMemory())
    I->setMetadata("llvm.mem.parallel_loop_access", L.getLoopID());

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1668
@@ +1667,2 @@
+    llvm::BasicBlock::iterator InsertPt) const;
+#endif
----------------
Same comment here (don't duplicate all of this, just use a boolean or some other mechanism).

http://reviews.llvm.org/D3644






More information about the cfe-commits mailing list