[PATCH] D136617: [opt] Print deprecation warning for use of legacy syntax with new pass manager

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/tools/opt/opt.cpp:756
+      errs() << PassList[0]->getPassArgument();
+      for (auto &P : drop_begin(PassList)) {
+        errs() << "," << P->getPassArgument();
----------------
bjope wrote:
> While giving the list of passes here might be helpful, maybe we should just say "please use opt -passes=<pipeline>" or something instead.
> Here are some (minor) concerns:
> 1) The suggested pipeline won't give identical behavior (e.g. if the test case that should be converted is some old bug reproducer it might be more interesting to use something more similar to what -print-pipeline-passes would output (including pass manager nestings etc).
> 2) The more text we output here (also including names of passes) the higher the risk that some test case would fail/pass incorrectly when FileCheck also looks at stderr.
> 
> But I don't know really. Maybe better to try to be helpful in the warning message anyway. One idea is to suggest using -print-pipeline-passes to get an idea for how the pipeline should look like, but that also makes the warning a bit longer.
1) the translation from `-pass1 -pass2` syntax to the actual new PM pipeline is already different from how the passes were run in the legacy PM, they're not interleaved with the new PM but they were with the legacy PM. interleaving passes is usually how pipelines should be run. it's possible that we were testing some edge case before and we're missing coverage now, but I don't think it's a huge deal, especially since a lot of those bugs are pass manager bugs that aren't an issue with the new PM
2) if a test outputs this message, it shouldn't be using the legacy syntax anyway and should be updated

but anyway, made the message generic so it's less confusing if it doesn't actually work (e.g. `func-pass,module-pass`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136617