[PATCH] D44524: Add an analysis printer for must execute reasoning

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In https://reviews.llvm.org/D44524#1040018, @anna wrote:

> Just to note, other printer passes like print-memoryssa, print-lvi, print-predicate-info, we print the annotations as comments just above the IR (using the asm printer interface). It's easier to see which instruction we are referring to and quickly spot the loops they were part of , *if loops are properly annotated in the IR*. It would be useful when debugging large code base IR and spotting the missing cases.
>  IMO, the missing cases may be hard to spot in this printer. One workaround  is to print the number of loops where it's not guaranteed to execute.  That can be a separate add on after landing this as well.
>
> From a testing perspective of the utility, this version works well for me.


This is a good idea.  I'd copied the basic structure of this from the mem deref printer, but I like the annotation version better.  Mind if I submit this one (so that I can start getting some basic testing around this code), and then return to the annotation as a follow on?  I'll change over mem-deref while I'm at it.



================
Comment at: lib/Analysis/MustExecute.cpp:25
+    DenseMap<Value*, SmallVector<Loop*, 4> > MustExec;
+    SmallVector<Value *, 4> Ordering;
+
----------------
anna wrote:
> Any reason for 4? Given we are looking at all instructions within all loops that's guaranteed to execute, it can be a much larger number. 
4 is simply the initial size and chosen fairly randomly.  The SmallVector will resize if needed.  Suggestions?


================
Comment at: lib/Analysis/MustExecute.cpp:85
+  for (Value *V: Ordering) {
+    V->print(OS);
+    OS << "\t(mustexec in ";
----------------
anna wrote:
> I think we can use `printAsOperand` so that nameless values are handled properly.
Good idea!


================
Comment at: lib/Analysis/MustExecute.cpp:87
+    OS << "\t(mustexec in ";
+    bool first = true;
+    for (const Loop *L : MustExec.lookup(V)) {
----------------
anna wrote:
> Perhaps rename this to exactlyOneLoop and  define that as  `MustExec[V]->size()  == 1`  
Er, what?  I don't understand your comment.  The "first" variable is simply being used to join a set of strings with a comma in between.  


================
Comment at: test/Analysis/MustExecute/loop-header.ll:4
+; CHECK: Printing analysis 'Instructions which execute on loop entry' for function 'header_with_icf':
+; CHECK: The following are guaranteed to execute (for the respective loops):
+; CHECK:   %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]	(mustexec in loop)
----------------
anna wrote:
> From a usability perspective in large IR where multiple transforms already done, we will have print statements like this:
> ```
> ; %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]  (mustexec in loop.split.unroll.split.split.crit_edge.... , another loop header name etc)
> ```
> It seems difficult to read. If the usecase of the printer is to test existing code, it's fine.
> However, printing this from large IR can make it pretty difficult to parse.
> How about something like:
> ```
> ; %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]  (mustexec in 3 loops: <header names> of all 3 loops)
> ```
> This number 3 can be retrieved from `MustExec[V]->size()`
Good idea.  Will implement.


Repository:
  rL LLVM

https://reviews.llvm.org/D44524