[PATCH] D131960: [IR] Reimplement time tracing of NewPassManager by PassInstrumentation framework

[dongjunduo via Phabricator via llvm-commits]

dongjunduo marked an inline comment as done.
dongjunduo added inline comments.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:413
+class TimeProfilingPassesHandler {
+
+public:
----------------
myhsu wrote:
> nit: can we move private members here (or using struct instead)
> nit: can we move private members here (or using struct instead)




================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:413
+class TimeProfilingPassesHandler {
+
+public:
----------------
dongjunduo wrote:
> myhsu wrote:
> > nit: can we move private members here (or using struct instead)
> > nit: can we move private members here (or using struct instead)
> 
> 
That class is designed following the class PrintIRInstrumentation([[ https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Passes/StandardInstrumentations.h#L41 | ref ]])

I think it's best to keep the class structure consistent. : )


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1168
+    PassInstrumentationCallbacks &PIC) {
+  this->PIC = &PIC;
+  PIC.registerBeforeNonSkippedPassCallback(
----------------
myhsu wrote:
> nit: personally I would simply rename the local `PIC` rather than spelling out `this`
dtto the one above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131960