[PATCH] D35570: [ORE] Port TailRecursionElimination to the new API

[Adam Nemet via Phabricator via llvm-commits]

anemet accepted this revision.
anemet added a comment.
This revision is now accepted and ready to land.

LGTM once we clear the question below.  Thanks!



================
Comment at: lib/Transforms/Scalar/TailRecursionElimination.cpp:70
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
----------------
davide wrote:
> anemet wrote:
> > Remove this?
> Which one in particular? The build fails without `OptimizationDiagnosticInfo.h` included.
OK :(


================
Comment at: lib/Transforms/Scalar/TailRecursionElimination.cpp:305
+      // using namespace ore;
+      ORE->emit(OptimizationRemark(DEBUG_TYPE, "tailcallelim", CI)
+               << "marked as tail call candidate");
----------------
davide wrote:
> davide wrote:
> > anemet wrote:
> > > It's up to you but you may want to differentiate the remarks by name (i.e. not use the same "tailcalelim" name).  It's easier to do stats, etc.
> > What do you mean exactly? Change the second parameter to be an unique identifier *within* the same file? 
> > e.g. "tailcallelim" vs "tailcallreadnone"?
> > Fine, but please note that in this case it's less interesting as we're always applying the same transformation (i.e. set TailCall()).
> > In other passes, e.g. Value Numbering it may have more sense as we perform different kind of xforms (e.g. instruction removal, moving etc..)
> Still probably makes sense to differentiate why, although that should be always uniquely identified by the string (I guess stats don't really want to grep in text messages, so, OK).
Yes, exactly.  It allows to do stats based on the situation under we perform the optimization.  I.e. if a case never triggers we don't want to pay for the analysis, etc.

Looks like you convinced that it's worth using different names but then you didn't make the change... 


https://reviews.llvm.org/D35570