[PATCH] Ephemeral values for LLVM invariants

Philip Reames listmail at philipreames.com
Fri Aug 1 16:45:31 PDT 2014


Meta comment on patch structure: I think you'd do well to separate most of the per-pass changes into separate changes.  Get the infrastructure in, with one key example.  Then, fix the passes which want to use assumptions multiple times, then trace through the pass order fixing preservation and use at each pass as encountered.  

LGTM.  I'd still prefer you have each of the specific passes reviewed by someone knowledgeable of those sections.

================
Comment at: lib/Analysis/CodeMetrics.cpp:64
@@ +63,3 @@
+    bool FoundNEUse = false;
+    for (const User *I : V->users())
+      if (!EphValues.count(I)) {
----------------
This code still has issues where you'll miss eph values based on visit order.  This isn't a correctness problem, just possible missed optimizations.

================
Comment at: lib/Analysis/CodeMetrics.cpp:74
@@ +73,3 @@
+    // worth of work for each of its loops (and, in the common case, ephemeral
+    // values in the loop are likely due to @llvm.assume calls in the loop).
+    if (!L->contains(I->getParent()))
----------------
Arguably, we should stop looking at eph values outside the loop during the search, but that's minor.  They're likely to have other uses as well.  

================
Comment at: test/Transforms/Inline/ephemeral.ll:23
@@ +22,3 @@
+; CHECK-NOT: call i1 @inner
+define i1 @outer() optsize {
+   %r = call i1 @inner()
----------------
hfinkel at anl.gov wrote:
> Philip Reames wrote:
> > 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.  
> I could hard code the current threshold into the RUN line (that having been said, I adapted this from a similar test that will also need to be changed should the heuristic be altered).
> 
> I don't understand your second comment. If you don't account for the ephemeral values, then the extra instructions in this example prevent inlining. If you do account for them, then the inlining happens.
For the second comment, you need a test case that shows the inliner not inling unless the values are ephemeral.  Your current tests could be made to pass by unconditional inlining.  Arguably, this is an inliner test, not an ephemeral value test and might already exists.

http://reviews.llvm.org/D180






More information about the llvm-commits mailing list