[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Paul Kirth via Phabricator via cfe-commits]

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


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:157-158
+    // In these cases we should not emit any diagnostic related to misexpect.
+    if (NOps < 3)
+      return;
+
----------------
nickdesaulniers wrote:
> would an `assert` be more appropriate then?  If cases 1/2/3 above are all highly unlikely, I'd make this an assert rather than early return.
I'm not sure an assert is appropriate. There are several types of profiling metadata, such as the value profiling metadata, and the types other than branch weight will have different numbers of arguments.  

I'll look into the tags that fall under `MD_prof`to see if any have less than 3 operands, but right now I'm not sure. I do know that other places that read the MD_prof tags use early return rather than an assert though.

The two examples I based my conventions on were:
- InstrProf.cpp:987 where VP metadata is extracted after first checking the number of operands.

- Metadata.cpp:1345 where branch weights are extracted using a less defensive approach, and do not check the number of operands before checking the metadata tag.


================
Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:79
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind willreturn }
----------------
nickdesaulniers wrote:
> does the debug info need to be in all of these unit tests?  If you still have the C sources handy, `-g0` is extremely handy in generating more concise IR for unit tests.
For this one, I think you're right, the debug info isn't needed, since we are checking that a diagnostic is never emitted.

In the other cases, I wanted to verify that the diagnostic flags the correct line number. If it is OK for the clang based tests to handle that aspect, then I can try to refactor the LLVM tests to be less precise about the output they are checking, and remove the debug data from the IR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324