[PATCH] D115351: [Debugify] Port verify-debuginfo-preserve to NewPM

[Nikola Tesic via Phabricator via llvm-commits]

ntesic added a comment.

Thank you for the comments, @aeubanks.
My replies are inline.



================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:952
 PreservedAnalyses NewPMDebugifyPass::run(Module &M, ModuleAnalysisManager &) {
-  applyDebugifyMetadata(M, M.functions(),
-                        "ModuleDebugify: ", /*ApplyToMF*/ nullptr);
+  if (Mode == DebugifyMode::SyntheticDebugInfo)
+    applyDebugifyMetadata(M, M.functions(),
----------------
aeubanks wrote:
> should this and the other addition below be separate passes? doesn't look like the two modes share a lot
As I am aware, this was already discussed before the introduction of the "Original DebugInfo mode" (please, find the [[ https://lists.llvm.org/pipermail/llvm-dev/2020-June/142341.html | link to the discussion ]]) and an agreement was to keep the single pass with two modes.


================
Comment at: llvm/test/DebugInfo/verify-di-preserve.ll:15
+
+; RUN: opt %s -verify-each-debuginfo-preserve -O2 -enable-new-pm=false -disable-output 2>&1 | FileCheck --check-prefix=VERIFY-EACH-LEGACY-PM %s
+
----------------
aeubanks wrote:
> I'd rather not test the legacy PM, it's in the process of being removed, ditto for the other RUN lines
I agree, now that I am seeing some test platforms don't support '-flegacy-pass-manager', it would be best to test only default (new) PM.


================
Comment at: llvm/tools/opt/opt.cpp:206
 
 static cl::opt<bool> VerifyDebugInfoPreserve(
     "verify-debuginfo-preserve",
----------------
aeubanks wrote:
> we should move all these options into NewPMDriver.cpp and remove support for them in the legacy PM.
> can be a separate change though
If you agree, I would keep that as future work, to move those options for both SytheticDebugInfo and OriginalDebugInfo mode together.


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

https://reviews.llvm.org/D115351