[PATCH] D86360: Add new hidden option -print-changed which only reports changes to IR

[Jamie Schmeiser via Phabricator via llvm-commits]

jamieschmeiser added inline comments.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:322
+
+void handleInitialIR(Any IR, raw_ostream &Out) {
+  StringRef Banner("*** IR Dump At Start: ***");
----------------
ychen wrote:
> jamieschmeiser wrote:
> > ychen wrote:
> > > Why is the round-trip of defining these here and passing them into the ctor of IRChangePrinter? Why do we need private callbacks such as ChangePrinter::HandleInitialIR. It seems just a trivial wrapper of handleInitialIR.
> > The reason for the round-trip structure is that there are more instantiations of these templates that report changes in different forms still to come.  I will be subsequently posting reviews for at least 2 different change reporters based on these base classes.  By defining these templates in this fashion, the functionality of comparing different IR representations and determining when changes have occurred and should be filtered, reported or ignored is all in common in the base classes.  The derived classes will be called when interesting events happen and only need to handle presenting the events without having to determine when the events occur.
> Maybe I missed something. This sounds like either virtual inheritance or CRTP are better alternatives than many function objects as class members. I would prefer CRTP.
@ychen Sorry for the delay in response but I had to concentrate on preparing my tutorial for the meeting as the recording is due today.  Anyway, I don't really think CRTP is really applicable here because there really aren't polymorphic calls being made.  The calls are registered with the PIC and at that point, everything about the calls is essentially known so there isn't polymorphism in the calls.  There are 3 ways of handling the callbacks: lambdas (as I've done), virtual overrides and leaving the functions to be defined by the template instantiation.  Pre-C++11, I would have used virtual overrides and I think this is a clean way of handling this situation.  The bases would be abstract and the derived classes would just provide the overrides.  However, it seems that the llvm community shies away from virtual hierarchies in favour of CRTP which is appropriate for the way it is mostly used, but not in this situation IMHO.  Virtuals have a bad rap for being unefficient but would be fine here.  I don't like leaving functions that have to be provided by template instantiations as the errors provided when they are incorrect often come from the linking stage and it can be very difficult to determine how to properly provide the necessary functions in the instantiation.  Lambdas seem to be preferred over virtuals so I chose them.  I would be fine with changing the hierarchy to use virtual functions but I would shy away from requiring the instantiation from providing missing functions even though it would be a more efficient way of calling the functions since it makes it harder to extend the hierarchy.  Efficiency isn't the main concern here since this is for aiding in code development/understanding and the overhead of virtual calls or calling through function pointers (lambdas) is small in comparison to the overhead involved in saving the representation of the IR.  Would you prefer this to be expressed with virtual functions?  If so, I will change it but I would rather avoid leaving functions to be provided by instantiations.  I did that in one of the subsequent PRs and found it cumbersome and difficult.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86360/new/

https://reviews.llvm.org/D86360