[PATCH] Modularizing LICM

Nema, Ashutosh Ashutosh.Nema at amd.com
Sun Feb 1 21:00:17 PST 2015

Thanks Hal for reviewing this change.
Your suggestions looks fine, will work on it.

Bruno, Philip: 
If possible please review this change.


-----Original Message-----
From: hfinkel at anl.gov [mailto:hfinkel at anl.gov] 
Sent: Friday, January 30, 2015 9:15 PM
To: Nema, Ashutosh; listmail at philipreames.com; bruno.cardoso at gmail.com
Cc: hfinkel at anl.gov; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Modularizing LICM

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