tejohnson added inline comments.
================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:425
+ /// values in order to be included in misexpect diagnostics.
+ Optional<uint64_t> DiagnosticsMisExpectTollerance = 0;
+
----------------
s/Tollerance/Tolerance/ (and elsewhere throughout patch)
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1967
+ Diags.Report(diag::warn_drv_diagnostics_misexpect_requires_pgo)
+ << "-fdiagnostics-misexpect-tollerance=";
+ }
----------------
Add tests for expected errors?
================
Comment at: clang/test/Profile/misexpect-branch.c:25
+ int x = 0;
+ if (likely(rando % (outer_loop * inner_loop) == 0)) { // exact-warning-re
{{Potential performance regression from use of __builtin_expect(): Annotation
was correct on {{.+}}% ({{[0-9]+ / [0-9]+}}) of profiled executions.}}
+ x = baz(rando);
----------------
Can you add a case where an unlikely() is wrong?
================
Comment at: llvm/docs/MisExpect.rst:2
+===================
+Misexpect
+===================
----------------
paulkirth wrote:
> tejohnson wrote:
> > I don't see a mention of -Wmisexpect in the document. Should there be
> > user-facing clang documentation, this seems more specific to the LLVM
> > implementation?
> I will update the patch to include similar documentation this for clang.
>
> Additionally, my understanding is that documentation for warnings is
> auto-generated from the tablegen, so that at least will be available
> automatically.
>
>
Should the clang documentation already be added to this patch? I couldn't find
it.
================
Comment at: llvm/docs/MisExpect.rst:22
+The MisExpect checks in the LLVM backend follow a simple procedure: if the
+profiling counter associated with an instruction using the ``llvm.expect``
+intrinsic was too low along the expected path, then it emits a diagnostic
----------------
paulkirth wrote:
> tejohnson wrote:
> > Suggest using "profile weight" not profiling counter since we don't really
> > have profile counters associated with instructions (rather with edges in
> > the MST, at least in the IR PGO).
> >
> > Also, doesn't the profile weight being too low only handle the LIKELY case?
> > What about something being marked UNLIKELY with a hot/high profile weight?
> > edit: After looking through the implementation, I think the comparison only
> > being done in this direction is based on the way it is implemented, but it
> > is unclear from this documentation here how the comparison is handling
> > things being off in either direction.
> Maybe my mental model is off here, but doesn't the `llvm.expect` intrinsic
> mark a specific value as the 'likely' or expected value? So if you want to
> mark a branch as unlikely, you're essentially marking the other half of it as
> 'hot'.
>
> We could change the comparison to compare all parts of the branch weights
> too, but the case of an 'unlikely' branch being too hot in my model is
> captured by the 'likely' branch becoming too 'cold'.
>
> If that is incorrect, I'd really appreciate some guidance on how to model
> this more accurately.
Yes I agree that is how llvm.expect is implemented, and I can see from your
implementation that it should be handling that correctly. It was just unclear
from the way it is written here what was happening. I like the new wording
which is less specific to the way the checking is implemented.
================
Comment at: llvm/docs/MisExpect.rst:39
+``-unlikely-branch-weight`` LLVM options. During verification, if the
+profile count is less than the calculated threshold, then we will emit a
+remark or warning detailing a potential performance regression. The
----------------
paulkirth wrote:
> tejohnson wrote:
> > s/count/weight/
> >
> > Also, similar to my earlier comment, what about an expect UNLIKELY branch
> > with a high profile weight?
> I think I've reworded this to clarify things a bit more, but let me know if
> it still needs some polish.
I think the new version is good, thanks.
================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:12
+/// This file implements a simple parser to decode commandline option for
+/// misexpect tollerance that supports both int.
+///
----------------
Is the end of the sentence incomplete? I.e. should be "both int and (something
else)"?
================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:26
+// Parse misexpect tollerance argument value.
+// Valid option values are
+// integer: manually specified threshold
----------------
Fix line wrapping. But sentence could be simplified too - is this just saying
that the input must be an integer?
================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:42
+// A simple CL parser for '-misexpect-tollerance='
+class HotnessThresholdParser : public cl::parser<Optional<uint64_t>> {
+public:
----------------
Where is this class used?
================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:51
+ return O.error("Invalid argument '" + Arg +
+ "', only integer or 'auto' is supported.");
+
----------------
I don't see where 'auto' is handled?
================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1737
+ misexpect::checkExpectAnnotations(*TI, Weights, false);
+
----------------
Document const parameter (e.g. "/*IsFrontend=*/false"), here and in other calls.
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:78
+
+Instruction *getOprndOrInst(Instruction *I) {
+ assert(I != nullptr && "MisExpect target Instruction cannot be nullptr");
----------------
Suggest renaming to something more intuitive. E.g. getInstCondition?
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:180
+ // To determine our threshold value we need to obtain the branch probability
+ // for the weights added by llvm.expect and use that proportion to calcualte
+ // our threshold based on the colledted profile data.
----------------
typo in calculate
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:181
+ // for the weights added by llvm.expect and use that proportion to calcualte
+ // our threshold based on the colledted profile data.
+ const llvm::BranchProbability LikelyProbablilty(LikelyBranchWeight,
----------------
typo in collected
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:188
+ auto Tollerance = getMisExpectTollerance(I.getContext());
+ if (Tollerance < 0)
+ Tollerance = 0;
----------------
afaict this handling duplicates what is done in parseTolleranceOption when the
tolerance comes in from the clang option. Can you just do this handling once,
here?
================
Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:1
+; RUN: llvm-profdata merge %S/Inputs/misexpect-branch-correct.proftext -o
%t.profdata
+
----------------
Add a one sentence description at the top of the llvm tests about what they are
testing.
================
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
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115907/new/
https://reviews.llvm.org/D115907
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
