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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked an inline comment as done.
paulkirth added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:150
+    // Operand 0 is a string tag "branch_weights"
+    if (MDString *Tag = cast<MDString>(MD->getOperand(0))) {
+      unsigned NOps = MD->getNumOperands();
----------------
nickdesaulniers wrote:
> paulkirth wrote:
> > nickdesaulniers wrote:
> > > Sorry if I'm going back and forth on this, but it may make sense to check the number of operands first, before accessing any.
> > No problem. I've reintroduced the check. I was under the impression that metadata could not lack a 0th element, but even in that case I should have checked for additional elements. 
> > 
> > There should always be at least 2 branch weights, otherwise branch weights are not necessary, and we should never end up here.
> > 
> > Should I add a comment saying that?
> Sorry, I meant "check the lower bound of the operands first, ..." as `NOps` might be `0` which would cause `MD->getOperand(0)` to fail, and `RealWeights` to be size `-1`.  `if (Nops < 1 || Nops > 3) return;`
> 
> I think a comment would be helpful, too.
Thanks for the clarification and example. I think we're on the same page, but let me make sure, since I think I might have muddied the conversation by adding some incomplete context to my reasoning. Here is my reasoning in full, and a proposed comment. Let me know if there is something I'm missing, or that I've misunderstood.

I think that the check: `if(NOps <3) return;` is the correct semantics here. 
Operand 0 will always exist if `NOps >=3`.
In the case of a switch instruction  `NOps > 3` needs to be supported.

Any profiling data with only less than 2 branch weights implies that the target is completely deterministic, and should not have branch weights associated with it. However, even if that were that happen, we should not emit any misexpect warnings, 

Proposed comment:
```
// Only emit misexpect diagnostics if at least 2 branch weights are present. 
// Less than 2 branch weights means that the profiling metadata is incorrect, 
// is not branch weight metadata, or that the branch is completely deterministic. 
// In these cases we should not emit any diagnostic related to misexpect.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324