[PATCH] D90365: [Debugify] Port -debugify-each to NewPM

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D90365#2362878 <https://reviews.llvm.org/D90365#2362878>, @aeubanks wrote:

> In D90365#2362635 <https://reviews.llvm.org/D90365#2362635>, @MaskRay wrote:
>
>> In D90365#2362581 <https://reviews.llvm.org/D90365#2362581>, @aeubanks wrote:
>>
>>> Thanks for looking into this!
>>>
>>> I was previously worried that modifying the IR in pass instrumentation would be bad, but maybe it's ok? The biggest thing I was worried about was unintentionally invalidating analyses, but it looks like both debugify passes preserve all analyses, so it might be ok.
>>
>> Yeah I was a bit concerned as well. The code says `PreservedAnalyses::all()` so may be it is fine? The dirty part is the const_cast.
>>
>>> I'm not so sure of moving the flags into Debugify.cpp since they are debugging tools, but I'm not 100% against it.
>>
>>
>>
>>> DebugifyEachInstrumentation should probably be part of StandardInstrumentations so that all users of StandardInstrumentations will get it for free (tying into moving the flags to not be opt-specific). DebugifyEachInstrumentation::registerCallbacks can check if DebugifyEach is set or not to determine whether or not to actually register callbacks, which is cleaner than checking in NewPMDriver.cpp.
>>
>> -debugify-export writes a file. So even if it is moved to StandardInstrumentations, all the users (clang/opt newPM/opt legacyPM) will need to do something with the option.
>
> Ah I see, that makes sense.
>
> I still think DebugifyEachInstrumentation should be treated in StandardInstrumentations like the others, and DebugifyEachInstrumentation::registerCallbacks can check the flags to enable/disable itself.

Emmm. Since we don't really benefit from having the code StandardInstrumentations (using -debugify-export requires the knowledge about DebugifyStatsMap), having the code in Debugify.cpp is fine?
The implementation is actually all in Debugify.cpp . Having `DebugifyEachInstrumentation` can make StandardInstrumentations unaware of DebugInfo, which achieves good isolation.
This allows us to make apply* and check* `static` after legacy PM debugify code in opt.cpp is removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90365