nickdesaulniers 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();
----------------
paulkirth wrote:
> 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.
> ```
Got it, yep that should be good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66324/new/
https://reviews.llvm.org/D66324
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
