[PATCH] D31613: [LoopPeeling] Get rid of Phis that become invariant after N steps

[Anna Thomas via Phabricator via llvm-commits]

anna requested changes to this revision.
anna added a comment.
This revision now requires changes to proceed.

Hi Max,
Couple of comments inline. The logic as such looks correct to me.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:101
+    // Store pre-calculated values of Depth here.
+    DenseMap<PHINode *, unsigned> InvariantDepth;
+
----------------
You can use a `SmallDenseMap` here since number of phis within a single basic block (i.e. header) is generally a small number.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:153
+    // Now go through all Phis to calculate their depth. Max depth is the
+    // desired number of iteratios we want to peel.
+    unsigned MaxDepth = 0;
----------------
Nit: iterations.


================
Comment at: test/Transforms/LoopUnroll/peel-loop-not-forced.ll:90
+}
+
+; Peeling should fail due to method size.
----------------
Could you please add a test case that  exercises the logic of choosing the depth based on the code size? i.e. say we have 2 phi's each of which have maxDepth of 3 and 4, but due to the code size restriction on peeling, you need to peel by only 3. 


https://reviews.llvm.org/D31613