[PATCH] Ephemeral values for LLVM invariants

[Philip Reames]

I don't spot any obvious bugs, but I don't feel comfortable signing off on this code either.  You need to find a reviewer who is familiar with this code.

Also, your tests need improvement.  Some points mentioned inline.  You're testing for the impact on assume, but not for the impact on the other intrinsics.

================
Comment at: lib/Analysis/CodeMetrics.cpp:47
@@ +46,3 @@
+        case Intrinsic::var_annotation:
+          // These intrinsics don't actually represent code after lowering.
+          WorkSet.push_back(CI);
----------------
Most of this change is driven by the assume intrinsic changes, but this actually changes behaviour for existing code as well.  It might be good to separate this infrastructure into two distinct changes and get the addition of the other intrinsics more widely reviewed.  (i.e. despite the title and description, this *isn't* specific to assume/invariant)

I'm mostly suggesting this for risk management and blame purposes.  

================
Comment at: lib/Analysis/CodeMetrics.cpp:56
@@ +55,3 @@
+  SmallPtrSet<const Value *, 32> Visited;
+
+  while (!WorkSet.empty()) {
----------------
A useful optimization might be to seed the EphValues set with every single use value in WorkSet.  This would help cases like:
a = icmp
assume(a)
c = and a, b
assume(c)

With your current implementation, if you consider 'a' first, you'll discard it because 'c' is not known ephemeral.  You'll eventually revisit it (through 'c''s operands), but this is wasted work.  

You could even go a step further and make the worklist include only items just found to be ephemeral.  You'd essentially be growing the frontier of ephemeral values back through the value graph.  This might be a more obvious algorithm.

Worth noting is that neither algorithm handles phi cycles.  Both are conservative, not optimistic.  (In the classic data flow sense.)  I think that's fine for now, but worth noting explicitly. 

================
Comment at: lib/Analysis/IPA/InlineCost.cpp:899
@@ +898,3 @@
+    // Skip ephemeral values.
+    if (EphValues.count(I))
+      continue;
----------------
It looks like the previous check is now redundant with this one?  It might be worth leaving it in place for clarity, but if so, an explicit comment might be warranted.  

================
Comment at: test/Transforms/Inline/ephemeral.ll:23
@@ +22,3 @@
+; CHECK-NOT: call i1 @inner
+define i1 @outer() optsize {
+   %r = call i1 @inner()
----------------
This test appears really fragile w.r.t. our current inlining heuristics.  Is there a way you could restructure this to avoid that?  If not, add some comments explaining how to adjust the test if the inlining heuristic change.  

You also need to add some form of negative example to show that the ephemeral logic actually contributes to the inlining.  

================
Comment at: test/Transforms/LoopUnroll/ephemeral.ll:7
@@ +6,3 @@
+
+define i32 @test1(i32* nocapture %a) nounwind uwtable readonly {
+entry:
----------------
Same comments as above w.r.t. test structure.

Also, a comment explaining the test approach would be helpful.  It looks like you're relying on the return to be constant folded after unrolling?

http://reviews.llvm.org/D180