[PATCH] Modularizing LICM

hfinkel at anl.gov hfinkel at anl.gov
Fri Jan 30 07:45:20 PST 2015

I think this generally looks reasonable.

Please don't expose the MayThrow and HeaderMayThrow variables without also pulling out a utility routine to compute them (making a function out of the relevant part of LICM::runOnLoop is fine). Then encapsulate them into a structure. We could call this something like LICMSafetyInfo, and the routine could be called computeLICMSafetyInfo. These are really internal details, and I don't want multiple callers to replicate their logic, and we might want to extend this preprocessing in the future.


Comment at: include/llvm/Transforms/Utils/LoopUtils.h:17
@@ -16,1 +16,3 @@
+#include "llvm/Analysis/AliasSetTracker.h"
+#include "llvm/IR/PredIteratorCache.h"
Don't include this header. Just add a forward declaration below.

Comment at: include/llvm/Transforms/Utils/LoopUtils.h:18
@@ -17,1 +17,3 @@
+#include "llvm/Analysis/AliasSetTracker.h"
+#include "llvm/IR/PredIteratorCache.h"
 namespace llvm {
Same here (it is only used by reference below).

Comment at: include/llvm/Transforms/Utils/LoopUtils.h:78
@@ +77,3 @@
+                const DataLayout *, TargetLibraryInfo *, Loop *,
+                AliasSetTracker *, bool , bool);
Please include variable names here, leaving out the ones that are obvious from the type is fine, but the scalar parameters should have them for clarity.

(same for the ones below)

[Given that I'd like these parameters encapsulated, this comment is just for general reference]

Comment at: lib/Transforms/Scalar/LICM.cpp:983
@@ +982,3 @@
+static bool pointerInvalidatedByLoop(Value *V, uint64_t Size,
+                              const AAMDNodes &AAInfo, AliasSetTracker *CurAST) {
+  // Check to see if any of the basic blocks in CurLoop invalidate *V.
Indenting looks odd here.



More information about the llvm-commits mailing list