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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Analysis/MustExecute.cpp:87
+    OS << "\t(mustexec in ";
+    bool first = true;
+    for (const Loop *L : MustExec.lookup(V)) {
----------------
reames wrote:
> anna wrote:
> > reames wrote:
> > > 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.  
> > What I meant is we can do something like this instead of using the first variable:
> > ```
> > const bool exactlyOneLoop = MustExec[V]->size()  == 1;
> > for (const Loop *L : MustExec.lookup(V)) {
> >       OS << L->getHeader()->getName();
> >       if (!ExactlyOneLoop)
> >           OS << ", ";
> > }
> > ```
> > 
> Ah, I see where you're going.  And actually you're pointing out a bug in the code.  What I'd actually wanted was: "name, name, name".  What I'd wrote produced "name name, name, "  Your suggestion is a cleanup of the code as written, but doesn't work once I fix the formatting bug. 
I didn't notice the bug originally, but I think the cleanup should fix the bug :)
We should produce "name, name, name" because the boolean `exactlyOneLoop` is a constant and set outside the loop. We don't change it (unlike the `first` variable which was changed in the loop, and was the reason for the bug).


Repository:
  rL LLVM

https://reviews.llvm.org/D44524