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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90365