[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked 5 inline comments as done.
paulkirth added inline comments.


================
Comment at: llvm/docs/MisExpect.rst:54
+
+.. option:: -pgo-warn-misexpect
+
----------------
tejohnson wrote:
> paulkirth wrote:
> > tejohnson wrote:
> > > Looking at the code, the -pgo-warn-misexpect seems to be useful for internal LLVM testing via 'opt', to mimic -Wmisexpect, so it probably should be documented as such.
> > Yes, its useful to test llvm, but shouldn't `opt` be usable in the same way? To me it seems useful for opt to support the warning directly too, but I'm happy to defer here if you think that's confusing or shouldn't be the case.
> Oh I'm not disagreeing with having the internal option and using it for opt, that's very useful. My comment was in the context of not having the user-facing clang documentation with the clang driver level option (which you since added). I was just suggesting you add a note that this option is an alternative to the clang option for use when e.g. testing via opt. Since there is now separate clang documentation I think it is less important now.
Thanks for the clarification.


================
Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:3
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s
+
----------------
tejohnson wrote:
> tejohnson wrote:
> > The old PM is gone in the optimization pipeline, so these old PM lines in this and other tests and the comment about the new PM can go away.
> I see you removed the NewPM-style invocations and kept the legacy ones - afaict opt will now convert the legacy style pass invocations to the new PM, but I think it is preferable to use the New PM style invocations.
yes, I misread your earlier comment.  I have restored the New PM style opt invocations, and removed the legacy PM ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907