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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri reopened this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Reverted in rG6861d938e5c946cc7079d9849ef7560d07aa2d80 
, looking 
forward to the rework!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D66324#2395230 , @haowei wrote:

> In D66324#2394687 , @lebedev.ri 
> wrote:
>
>> In D66324#2336555 , @lebedev.ri 
>> wrote:
>>
>>> In D66324#2336186 , @phosek wrote:
>>>
 I apologize for the late response, I somehow missed the earlier responses. 
 We have successfully used this feature in Fuchsia and found it useful, but 
 I agree that the issues raised need to be addressed. Unfortunately 
 @paulkirth is no longer working on this project. I hope that someone from 
 our team can take a look but it might take a few weeks. If you prefer, we 
 could revert this change and then reland an improved version in the future?
>>>
>>> I would very much prefer *NOT* not revert if someone is going to step up to 
>>> work on these problems soon (within next 4 weeks?).
>>>
>>> That being said, in light of that bug, my original doubts about the 
>>> underlying data type (a novel `MD_misexpect`,
>>> with structure different from `MD_prof`) have reappeared with double 
>>> strength. I really think they should share underlying type.
>>
>> I'll be posting a revert soon.
>
> Sorry for the late reply. Since @paulkirth is no longer working on this 
> project. I can take over it and start working on it full time beginning next 
> week. It would take me some time to get familiar with the code and work on 
> the fix though. If you still prefer reverting this change, we can work on an 
> improved version and reland it in the future.

I think revert will result in an more easily reviewable fix, yes.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haowei Wu via Phabricator via cfe-commits]

haowei added a comment.

In D66324#2394687 , @lebedev.ri wrote:

> In D66324#2336555 , @lebedev.ri 
> wrote:
>
>> In D66324#2336186 , @phosek wrote:
>>
>>> I apologize for the late response, I somehow missed the earlier responses. 
>>> We have successfully used this feature in Fuchsia and found it useful, but 
>>> I agree that the issues raised need to be addressed. Unfortunately 
>>> @paulkirth is no longer working on this project. I hope that someone from 
>>> our team can take a look but it might take a few weeks. If you prefer, we 
>>> could revert this change and then reland an improved version in the future?
>>
>> I would very much prefer *NOT* not revert if someone is going to step up to 
>> work on these problems soon (within next 4 weeks?).
>>
>> That being said, in light of that bug, my original doubts about the 
>> underlying data type (a novel `MD_misexpect`,
>> with structure different from `MD_prof`) have reappeared with double 
>> strength. I really think they should share underlying type.
>
> I'll be posting a revert soon.

Sorry for the late reply. Since @paulkirth is no longer working on this 
project. I can take over it and start working on it full time beginning next 
week. It would take me some time to get familiar with the code and work on the 
fix though. If you still prefer reverting this change, we can work on an 
improved version and reland it in the future.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D66324#2336555 , @lebedev.ri wrote:

> In D66324#2336186 , @phosek wrote:
>
>> I apologize for the late response, I somehow missed the earlier responses. 
>> We have successfully used this feature in Fuchsia and found it useful, but I 
>> agree that the issues raised need to be addressed. Unfortunately @paulkirth 
>> is no longer working on this project. I hope that someone from our team can 
>> take a look but it might take a few weeks. If you prefer, we could revert 
>> this change and then reland an improved version in the future?
>
> I would very much prefer *NOT* not revert if someone is going to step up to 
> work on these problems soon (within next 4 weeks?).
>
> That being said, in light of that bug, my original doubts about the 
> underlying data type (a novel `MD_misexpect`,
> with structure different from `MD_prof`) have reappeared with double 
> strength. I really think they should share underlying type.

I'll be posting a revert soon.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D66324#2336186 , @phosek wrote:

> I apologize for the late response, I somehow missed the earlier responses. We 
> have successfully used this feature in Fuchsia and found it useful, but I 
> agree that the issues raised need to be addressed. Unfortunately @paulkirth 
> is no longer working on this project. I hope that someone from our team can 
> take a look but it might take a few weeks. If you prefer, we could revert 
> this change and then reland an improved version in the future?

I would very much prefer *NOT* not revert if someone is going to step up to 
work on these problems soon (within next 4 weeks?).

That being said, in light of that bug, my original doubts about the underlying 
data type (a novel `MD_misexpect`,
with structure different from `MD_prof`) have reappeared with double strength. 
I really think they should share underlying type.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

I apologize for the late response, I somehow missed the earlier responses. We 
have successfully used this feature in Fuchsia and found it useful, but I agree 
that the issues raised need to be addressed. Unfortunately @paulkirth is no 
longer working on this project. I hope that someone from our team can take a 
look but it might take a few weeks. If you prefer, we could revert this change 
and then reland an improved version in the future?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

+1 for revert


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: thakis.
lebedev.ri added a comment.
Herald added a subscriber: wenlei.

@paulkirth @phosek @leonardchan @thakis @nickdesaulniers
It seems to me that this feature is still broken an unusable - 
https://bugs.llvm.org/show_bug.cgi?id=45073 -and the bug was reported more than 
half a year ago.
If no one is willing to step up and fix it, the i can only assume that no one 
is using this and or cares, and this broken incomplete feature should be 
reverted.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: cfe/trunk/lib/Frontend/CompilerInvocation.cpp:3457
+  if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
+

rnk wrote:
> Clang generally tries to avoid relying on LLVM option parsing if at all 
> possible. It is not thread-safe, among other things. This causes essentially 
> every compile to call `llvm::cl::ParseCommandLineOptions`, when previously it 
> would only happen if the user passed unstable -mllvm flags.
And it is buggy, if you add "!" as a fix, there are many test failures...
http://45.33.8.238/linux/18098/step_7.txt


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid "Away June-Sep" Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: cfe/trunk/lib/Frontend/CompilerInvocation.cpp:3457
+  if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
+

Clang generally tries to avoid relying on LLVM option parsing if at all 
possible. It is not thread-safe, among other things. This causes essentially 
every compile to call `llvm::cl::ParseCommandLineOptions`, when previously it 
would only happen if the user passed unstable -mllvm flags.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov added a comment.

Looks like the issue is fixed in 
https://github.com/llvm/llvm-project/commit/6d2b75e0887ee87e247756c4d51733616bb2f356


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jean-Michaël Celerier via Phabricator via cfe-commits]

jcelerier added a comment.

Also hit by this issue - a lot more people are certainly going to have it as 
this has started landing in Linux distros.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

@xbolva00 I could, but to me this is not trivial (I'm not familiar with the 
code and I'm not sure what I would break). If it's easy for you, then please do 
(otherwise I can send a patch for review).
I have already worked around this issue in a different way (by running the 
compiler invocation in a separate process).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Fix is easy,  can you commit it as obvious?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ayke via Phabricator via cfe-commits]

aykevl added a comment.

I have the same issue as @wenju. The second time I call 
`ExecuteCompilerInvocation` it will give the error above, leading me to believe 
some memory isn't properly cleared.

Note: the `--pgo-warn-misexpect` option is not passed in the compiler 
invocation.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Wenju He via Phabricator via cfe-commits]

wenju added a comment.

We see error "clang (LLVM option parsing): for the   --pgo-warn-misexpect 
option: may only occur zero or one times!" when 
clang::CompilerInvocation::CreateFromArgs and clang::ExecuteCompilerInvocation 
are called twice.

This line: if (Diags.isIgnored(diag::warn_profile_data_misexpect, 
SourceLocation()))
I think it should be !Diags.isIgnored.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: llvm/trunk/include/llvm/IR/DiagnosticInfo.h:79
+  DK_FirstPluginKind,
+  DK_MisExpect
 };

paulkirth wrote:
> jrtc27 wrote:
> > Is this not going to clash with the first caller to 
> > `getNextAvailablePluginDiagnosticKind`? `DK_FirstPluginKind` is special and 
> > shouldn't have anything after it.
> hmm, I wasn't aware of that. Is this documented somewhere? Also no tests seem 
> to break from this, so that might be another area to address.
> 
> I'll upload a patch for this that moves the flag, and adds a note about the 
> ordering.
Please see D67648 for future reference of this defect.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.



Comment at: llvm/trunk/include/llvm/IR/DiagnosticInfo.h:79
+  DK_FirstPluginKind,
+  DK_MisExpect
 };

jrtc27 wrote:
> Is this not going to clash with the first caller to 
> `getNextAvailablePluginDiagnosticKind`? `DK_FirstPluginKind` is special and 
> shouldn't have anything after it.
hmm, I wasn't aware of that. Is this documented somewhere? Also no tests seem 
to break from this, so that might be another area to address.

I'll upload a patch for this that moves the flag, and adds a note about the 
ordering.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/trunk/include/llvm/IR/DiagnosticInfo.h:79
+  DK_FirstPluginKind,
+  DK_MisExpect
 };

Is this not going to clash with the first caller to 
`getNextAvailablePluginDiagnosticKind`? `DK_FirstPluginKind` is special and 
shouldn't have anything after it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371635: Reland clang-misexpect: Profile Guided 
Validation of Performance Annotations… (authored by phosek, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D66324?vs=219728=219733#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

Files:
  cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/lib/CodeGen/CodeGenAction.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-branch.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default-only.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-nonconst.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch.proftext
  cfe/trunk/test/Profile/misexpect-branch-cold.c
  cfe/trunk/test/Profile/misexpect-branch-nonconst-expected-val.c
  cfe/trunk/test/Profile/misexpect-branch-unpredictable.c
  cfe/trunk/test/Profile/misexpect-branch.c
  cfe/trunk/test/Profile/misexpect-switch-default.c
  cfe/trunk/test/Profile/misexpect-switch-nonconst.c
  cfe/trunk/test/Profile/misexpect-switch-only-default-case.c
  cfe/trunk/test/Profile/misexpect-switch.c
  llvm/trunk/include/llvm/IR/DiagnosticInfo.h
  llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
  llvm/trunk/include/llvm/IR/MDBuilder.h
  llvm/trunk/include/llvm/Transforms/Utils/MisExpect.h
  llvm/trunk/lib/IR/DiagnosticInfo.cpp
  llvm/trunk/lib/IR/MDBuilder.cpp
  llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
  llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/trunk/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
  llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
  llvm/trunk/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/trunk/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/trunk/lib/IR/MDBuilder.cpp
===
--- llvm/trunk/lib/IR/MDBuilder.cpp
+++ llvm/trunk/lib/IR/MDBuilder.cpp
@@ -309,3 +309,15 @@
   };
   return MDNode::get(Context, Vals);
 }
+
+MDNode *MDBuilder::createMisExpect(uint64_t Index, uint64_t LikleyWeight,
+   uint64_t UnlikleyWeight) {
+  auto *IntType = Type::getInt64Ty(Context);
+  Metadata *Vals[] = {
+  createString("misexpect"),
+  createConstant(ConstantInt::get(IntType, Index)),
+  createConstant(ConstantInt::get(IntType, LikleyWeight)),
+  createConstant(ConstantInt::get(IntType, UnlikleyWeight)),
+  };
+  return MDNode::get(Context, Vals);
+}
Index: llvm/trunk/lib/IR/DiagnosticInfo.cpp
===
--- llvm/trunk/lib/IR/DiagnosticInfo.cpp
+++ llvm/trunk/lib/IR/DiagnosticInfo.cpp
@@ -370,5 +370,16 @@
   return OS.str();
 }
 
+DiagnosticInfoMisExpect::DiagnosticInfoMisExpect(const Instruction *Inst,
+ Twine )
+: DiagnosticInfoWithLocationBase(DK_MisExpect, DS_Warning,
+ *Inst->getParent()->getParent(),
+ Inst->getDebugLoc()),
+  Msg(Msg) {}
+
+void DiagnosticInfoMisExpect::print(DiagnosticPrinter ) const {
+  DP << getLocationStr() << ": " << getMsg();
+}
+
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
Index: llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
===
--- llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
+++ llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
@@ -40,6 +40,7 @@
   LowerSwitch.cpp
   Mem2Reg.cpp
   MetaRenamer.cpp
+  MisExpect.cpp
   ModuleUtils.cpp
   NameAnonGlobals.cpp
   PredicateInfo.cpp
Index: llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
===
--- llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
+++ llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
@@ -0,0 +1,177 @@
+//===--- MisExpect.cpp - 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 219728.
paulkirth added a comment.

Revert mismerge when landing.


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch-unpredictable.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% (0 / 8112) of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% (0 / 8112) of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

In D66324#1665784 , @lebedev.ri wrote:

> In D66324#1665773 , @gribozavr wrote:
>
> > Reverted in r371598.
> >
> > Another concern that I have is cross-compilation. LLVM's ADT is not set up 
> > to be cross-compiled like the rest of compiler-rt is.
>
>
> Uhm, i have a better question still - why xxhash is even there? it's not used 
> in the diff, and was not reviewed, it just magically appeared in the last 
> update of the patch:
>  https://reviews.llvm.org/D66324?vs=219617=219645


It shouldn't have. My last change only modified a test file to not trigger a 
memory corruption bug. I'm filing a bug for that, but am waiting on a login for 
the bug tracker.

What I suspect happened is that when this landed some local changes got sucked 
in. I'm terribly sorry, it wasn't my intention to add anything significant to 
the patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: compiler-rt/trunk/lib/profile/xxhash.h:41-42
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+

paulkirth wrote:
> chandlerc wrote:
> > Sorry folks, but you can't do this.
> > 
> > You can't depend on ADT from compiler-rt currently.
> > 
> > There are at least two problems here:
> > 
> > First problem is that this pollutes the profile library with symbols from 
> > ADT. That really doesn't seem reasonable without *significant* and invasive 
> > changes to ADT. Otherwise building LLVM and linking it with the profile 
> > library will create an ODR violation (imagine different assert levels or 
> > different versions of LLVM buing built and the host toolchain).
> > 
> > 
> > Second, and much more critically, we haven't gotten to 100% relicensed on 
> > ADT, so it is critical that we not depend on it from runtime libraries.
> > 
> > Third, a lot of this code seems to use old license headers. Please do not 
> > add any code like this to LLVM, and instead use the new LLVM license for 
> > all new code.
> > 
> > For now, this patch (and any related patches) need to be reverted until 
> > these are addressed. Especially the license issues.
> Sorry, this looks like a mismerge somehow. My patch should't have anything 
> from compiler-rt. I think maybe a local change got rolled in when Petr landed 
> my patch.
Sorry about that, this was my mistake, it seems two of my stales files got 
included in when I applied the patch locally. I'll revert the change and reland 
it correctly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: compiler-rt/trunk/lib/profile/xxhash.h:41-42
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+

chandlerc wrote:
> Sorry folks, but you can't do this.
> 
> You can't depend on ADT from compiler-rt currently.
> 
> There are at least two problems here:
> 
> First problem is that this pollutes the profile library with symbols from 
> ADT. That really doesn't seem reasonable without *significant* and invasive 
> changes to ADT. Otherwise building LLVM and linking it with the profile 
> library will create an ODR violation (imagine different assert levels or 
> different versions of LLVM buing built and the host toolchain).
> 
> 
> Second, and much more critically, we haven't gotten to 100% relicensed on 
> ADT, so it is critical that we not depend on it from runtime libraries.
> 
> Third, a lot of this code seems to use old license headers. Please do not add 
> any code like this to LLVM, and instead use the new LLVM license for all new 
> code.
> 
> For now, this patch (and any related patches) need to be reverted until these 
> are addressed. Especially the license issues.
Sorry, this looks like a mismerge somehow. My patch should't have anything from 
compiler-rt. I think maybe a local change got rolled in when Petr landed my 
patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D66324#1665773 , @gribozavr wrote:

> Reverted in r371598.
>
> Another concern that I have is cross-compilation. LLVM's ADT is not set up to 
> be cross-compiled like the rest of compiler-rt is.


Uhm, i have a better question still - why xxhash is even there? it's not used 
in the diff, and was not reviewed, it ajust magically appeared in the last 
update of the patch:
https://reviews.llvm.org/D66324?vs=219617=219645


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Reverted in r371598.

Another concern that I have is cross-compilation. LLVM's ADT is not set up to 
be cross-compiled like the rest of compiler-rt is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: compiler-rt/trunk/lib/profile/xxhash.h:41-42
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+

Sorry folks, but you can't do this.

You can't depend on ADT from compiler-rt currently.

There are at least two problems here:

First problem is that this pollutes the profile library with symbols from ADT. 
That really doesn't seem reasonable without *significant* and invasive changes 
to ADT. Otherwise building LLVM and linking it with the profile library will 
create an ODR violation (imagine different assert levels or different versions 
of LLVM buing built and the host toolchain).


Second, and much more critically, we haven't gotten to 100% relicensed on ADT, 
so it is critical that we not depend on it from runtime libraries.

Third, a lot of this code seems to use old license headers. Please do not add 
any code like this to LLVM, and instead use the new LLVM license for all new 
code.

For now, this patch (and any related patches) need to be reverted until these 
are addressed. Especially the license issues.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371584: clang-misexpect: Profile Guided Validation of 
Performance Annotations in LLVM (authored by phosek, committed by ).
Herald added a subscriber: delcypher.

Changed prior to commit:
  https://reviews.llvm.org/D66324?vs=219617=219645#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

Files:
  cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/lib/CodeGen/CodeGenAction.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-branch.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default-only.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-nonconst.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch.proftext
  cfe/trunk/test/Profile/misexpect-branch-cold.c
  cfe/trunk/test/Profile/misexpect-branch-nonconst-expected-val.c
  cfe/trunk/test/Profile/misexpect-branch-unpredictable.c
  cfe/trunk/test/Profile/misexpect-branch.c
  cfe/trunk/test/Profile/misexpect-switch-default.c
  cfe/trunk/test/Profile/misexpect-switch-nonconst.c
  cfe/trunk/test/Profile/misexpect-switch-only-default-case.c
  cfe/trunk/test/Profile/misexpect-switch.c
  compiler-rt/trunk/lib/profile/xxhash.c
  compiler-rt/trunk/lib/profile/xxhash.h
  llvm/trunk/include/llvm/IR/DiagnosticInfo.h
  llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
  llvm/trunk/include/llvm/IR/MDBuilder.h
  llvm/trunk/include/llvm/Transforms/Utils/MisExpect.h
  llvm/trunk/lib/IR/DiagnosticInfo.cpp
  llvm/trunk/lib/IR/MDBuilder.cpp
  llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
  llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/trunk/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
  llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
  llvm/trunk/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/trunk/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/trunk/include/llvm/IR/MDBuilder.h
===
--- llvm/trunk/include/llvm/IR/MDBuilder.h
+++ llvm/trunk/include/llvm/IR/MDBuilder.h
@@ -16,6 +16,7 @@
 
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/DataTypes.h"
 #include 
@@ -75,6 +76,10 @@
   /// Return metadata containing the section prefix for a function.
   MDNode *createFunctionSectionPrefix(StringRef Prefix);
 
+  /// return metadata containing expected value
+  MDNode *createMisExpect(uint64_t Index, uint64_t LikelyWeight,
+  uint64_t UnlikelyWeight);
+
   //===--===//
   // Range metadata.
   //===--===//
Index: llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
===
--- llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
+++ llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
@@ -39,3 +39,4 @@
 LLVM_FIXED_MD_KIND(MD_access_group, "llvm.access.group", 25)
 LLVM_FIXED_MD_KIND(MD_callback, "callback", 26)
 LLVM_FIXED_MD_KIND(MD_preserve_access_index, "llvm.preserve.access.index", 27)
+LLVM_FIXED_MD_KIND(MD_misexpect, "misexpect", 28)
Index: llvm/trunk/include/llvm/IR/DiagnosticInfo.h
===
--- llvm/trunk/include/llvm/IR/DiagnosticInfo.h
+++ llvm/trunk/include/llvm/IR/DiagnosticInfo.h
@@ -75,7 +75,8 @@
   DK_MIRParser,
   DK_PGOProfile,
   DK_Unsupported,
-  DK_FirstPluginKind
+  DK_FirstPluginKind,
+  DK_MisExpect
 };
 
 /// Get the next available kind ID for a plugin diagnostic.
@@ -1002,6 +1003,25 @@
   void print(DiagnosticPrinter ) const override;
 };
 
+/// Diagnostic information for MisExpect analysis.
+class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
+public:
+DiagnosticInfoMisExpect(const 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 219617.
paulkirth added a comment.

Add a new profdata file to use w/ misexpect-nonconst-switch.c

ASAN exposed an issue where a function may hash the same even if the number of 
counters is different. This means that when a PGO profile is read in, it is 
possible for an out of bounds write to occur due to a mismatch in the number of 
statements that require profile counters, and the number of counters 
initialized from the profile that was read in.

This change should allow the misexpect patch to reland w/o issue.


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch-unpredictable.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance 

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

[Petr Hosek via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371484: clang-misexpect: Profile Guided Validation of 
Performance Annotations in LLVM (authored by phosek, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D66324?vs=219219=219472#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66324

Files:
  cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/lib/CodeGen/CodeGenAction.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-branch.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default-only.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch-default.proftext
  cfe/trunk/test/Profile/Inputs/misexpect-switch.proftext
  cfe/trunk/test/Profile/misexpect-branch-cold.c
  cfe/trunk/test/Profile/misexpect-branch-nonconst-expected-val.c
  cfe/trunk/test/Profile/misexpect-branch-unpredictable.c
  cfe/trunk/test/Profile/misexpect-branch.c
  cfe/trunk/test/Profile/misexpect-switch-default.c
  cfe/trunk/test/Profile/misexpect-switch-nonconst.c
  cfe/trunk/test/Profile/misexpect-switch-only-default-case.c
  cfe/trunk/test/Profile/misexpect-switch.c
  llvm/trunk/include/llvm/IR/DiagnosticInfo.h
  llvm/trunk/include/llvm/IR/FixedMetadataKinds.def
  llvm/trunk/include/llvm/IR/MDBuilder.h
  llvm/trunk/include/llvm/Transforms/Utils/MisExpect.h
  llvm/trunk/lib/IR/DiagnosticInfo.cpp
  llvm/trunk/lib/IR/MDBuilder.cpp
  llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
  llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/trunk/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/trunk/lib/Transforms/Utils/CMakeLists.txt
  llvm/trunk/lib/Transforms/Utils/MisExpect.cpp
  llvm/trunk/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/trunk/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/trunk/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/trunk/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/trunk/lib/IR/MDBuilder.cpp
===
--- llvm/trunk/lib/IR/MDBuilder.cpp
+++ llvm/trunk/lib/IR/MDBuilder.cpp
@@ -309,3 +309,15 @@
   };
   return MDNode::get(Context, Vals);
 }
+
+MDNode *MDBuilder::createMisExpect(uint64_t Index, uint64_t LikleyWeight,
+   uint64_t UnlikleyWeight) {
+  auto *IntType = Type::getInt64Ty(Context);
+  Metadata *Vals[] = {
+  createString("misexpect"),
+  createConstant(ConstantInt::get(IntType, Index)),
+  createConstant(ConstantInt::get(IntType, LikleyWeight)),
+  createConstant(ConstantInt::get(IntType, UnlikleyWeight)),
+  };
+  return MDNode::get(Context, Vals);
+}
Index: llvm/trunk/lib/IR/DiagnosticInfo.cpp
===
--- llvm/trunk/lib/IR/DiagnosticInfo.cpp
+++ llvm/trunk/lib/IR/DiagnosticInfo.cpp
@@ -370,5 +370,16 @@
   return OS.str();
 }
 
+DiagnosticInfoMisExpect::DiagnosticInfoMisExpect(const Instruction *Inst,
+ Twine )
+: DiagnosticInfoWithLocationBase(DK_MisExpect, DS_Warning,
+ *Inst->getParent()->getParent(),
+ Inst->getDebugLoc()),
+  Msg(Msg) {}
+
+void DiagnosticInfoMisExpect::print(DiagnosticPrinter ) const {
+  DP << getLocationStr() << ": " << getMsg();
+}
+
 void OptimizationRemarkAnalysisFPCommute::anchor() {}
 void OptimizationRemarkAnalysisAliasing::anchor() {}
Index: llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
===
--- llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
+++ llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
@@ -72,6 +72,7 @@
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/CallPromotionUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
+#include "llvm/Transforms/Utils/MisExpect.h"
 #include 
 #include 
 #include 
@@ -1446,6 +1447,8 @@
   }
 }
 
+misexpect::verifyMisExpect(TI, Weights, TI->getContext());
+
 uint64_t TempWeight;
 // Only set weights if there is at least one non-zero weight.
 // In any other case, let the analyzer 

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

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: clang/test/Profile/misexpect-branch-cold.c:4
+// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+

lebedev.ri wrote:
> paulkirth wrote:
> > lebedev.ri wrote:
> > > Is there a test where `-Wmisexpect` isn't present, to verify that it is 
> > > off-by-default?
> > We can add one, but is that necessary? Don't the tests for diagnostics 
> > cover those already?
> To clarify: i'm interested in the case where the PGO data is provided but 
> `-Wmisexpect` is *not* specified.
Actually, this is moot. That test already exists in misexpect-branch.c: line 6.

Since that file actually has a mismatched use of __builtin_expect(), it's a 
better candidate anyway.

```// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -verify=foo```


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/test/Profile/misexpect-branch-cold.c:4
+// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+

paulkirth wrote:
> lebedev.ri wrote:
> > Is there a test where `-Wmisexpect` isn't present, to verify that it is 
> > off-by-default?
> We can add one, but is that necessary? Don't the tests for diagnostics cover 
> those already?
To clarify: i'm interested in the case where the PGO data is provided but 
`-Wmisexpect` is *not* specified.


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.



Comment at: clang/test/Profile/misexpect-branch-cold.c:4
+// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+

lebedev.ri wrote:
> Is there a test where `-Wmisexpect` isn't present, to verify that it is 
> off-by-default?
We can add one, but is that necessary? Don't the tests for diagnostics cover 
those already?


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

Okay, thank you for the patience, this looks good to me now too.
Please update patch description/subject - this patch is not a standalone tool.




Comment at: clang/test/Profile/misexpect-branch-cold.c:4
+// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+

Is there a test where `-Wmisexpect` isn't present, to verify that it is 
off-by-default?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+  LLVMContext::MD_misexpect,
+  MDBuilder(CI->getContext())
+  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);

paulkirth wrote:
> lebedev.ri wrote:
> > paulkirth wrote:
> > > lebedev.ri wrote:
> > > > Why can't `LLVMContext::MD_prof` be reused?
> > > It didn't seem appropriate to me, but that can be changed if it is the 
> > > right thing to do. 
> > > 
> > > However, I don't see code elsewhere that loops over `MD_prof` metadata 
> > > checking its tag. I see lots of examples that check if the tag matches, 
> > > and then exits if it doesn't match. 
> > > 
> > > If I added "misexpect" as a new `MD_prof`tag, then code in places like 
> > > `extractProfTotalWeight(...)` in IR/Metadata.cpp would have to be 
> > > updated. I'm not sure how pervasive that pattern is, but I did  want to 
> > > avoid introducing systemic changes like that.
> > > 
> > > https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336
> > That's not exactly what i was asking.
> > I'm specifically talking about how `"misexpect"` and `"branch_weigths"` 
> > carry basically the same data,
> > are set basically in the same place always, with the difference that 
> > `"misexpect"` also has the switch case index.
> > This is both somewhat bloaty, and is prone to getting out of sync.
> > 
> > I have two questions:
> > 1. Can `"misexpect"` metadata simply not be invented, but can existing 
> > `LLVMContext::MD_misexpect` simply be used?
> > 2. Can `"misexpect"` be changed to not store the weights, but a reference 
> > to the `LLVMContext::MD_misexpect` metadata that will be emitted anyway?
> > 
> Sorry for the misunderstanding. 
> 
> I think what you are suggesting is to recreate the misexpect metadata from 
> the MD_prof "branch_weights metadata. Is that right?
> 
> I think that may be possible, but it will add complexity. While the metadata 
> approach is very straightforward. I think there is value in keeping this 
> simple, and then iterating on it. 
> 
> Also, LowerExpectIntrinsic.cpp has a comment near the top suggesting that the 
> LikelyBranchWeight and UnlikelyBranchWeights be hoisted to a common space. 
> This would significantly shrink the size of the misexpect metadata if it were 
> accessible directly. However, that is a change that should happen separately 
> from this patch, and might require further discussion to find the correct 
> shared space.
> 
> Ultimately, however, I do not see a way to completely remove some kind of 
> metadata tag, as we only need to perform misexpect checks when the intrinsic 
> was used -- which should be a small minority of cases. If there is another 
> way, then I am happy to hear it, but again, maybe that should be another 
> patch.
> 
> 
> 
> nonnull

Yes.

> ...

Well, alright i guess, please add a note that this was at least considered into 
the patch description.



Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:141
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect

paulkirth wrote:
> lebedev.ri wrote:
> > Here and elsewhere:
> > is `!prof !1` no longer present?
> The misexpect metadata tags take those slots.
> 
> `br i1 %tobool, label %if.then, label %if.end, !prof !0, !misexpect !1`
> 
> Unless my understanding of how the tags work is flawed, I think this is the 
> expected behavior
I was worried that here the `!prof` were no longer emitted, but only 
`!misexpect` were emitted.
This seems not to be the case, so all good i think.


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 219219.
paulkirth added a comment.

Improve diagnostic output with profile counts


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch-unpredictable.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% (0 / 8112) of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% (0 / 8112) of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218200.
paulkirth added a comment.

Add clang and LLVM tests for __builtin_unpredictable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch-unpredictable.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-unpredictable.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218198.
paulkirth added a comment.

Add inline checks for misexpect tags to LowerExpectIntrinstic test


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: warning: 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218197.
paulkirth added a comment.

Update LowerExpectIntrinsic/basic.ll to generate misexpect metadata for a switch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of 

[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/test/Transforms/LowerExpectIntrinsic/basic.ll:141
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect

lebedev.ri wrote:
> Here and elsewhere:
> is `!prof !1` no longer present?
The misexpect metadata tags take those slots.

`br i1 %tobool, label %if.then, label %if.end, !prof !0, !misexpect !1`

Unless my understanding of how the tags work is flawed, I think this is the 
expected behavior



Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:292-293
+; CHECK: !3 = !{!"misexpect", i64 1, i64 2000, i64 1}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}

lebedev.ri wrote:
> Should there be a FIXME, are some `misexpect` missing here?
This is happenstance unfortunately.

```
; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
```

In both of these cases the expected index is still 0 or 1, so the metadata tags 
are deduplicated. 

If we want to check further, then we need to modify the test to select an index 
greater than 1.


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3449
 
+  if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");

lebedev.ri wrote:
> Err, this works? I'd expect `!` to be missing here..
I ran check-clang and check-llvm. Are there more tests I should be running?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+  LLVMContext::MD_misexpect,
+  MDBuilder(CI->getContext())
+  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);

lebedev.ri wrote:
> paulkirth wrote:
> > lebedev.ri wrote:
> > > Why can't `LLVMContext::MD_prof` be reused?
> > It didn't seem appropriate to me, but that can be changed if it is the 
> > right thing to do. 
> > 
> > However, I don't see code elsewhere that loops over `MD_prof` metadata 
> > checking its tag. I see lots of examples that check if the tag matches, and 
> > then exits if it doesn't match. 
> > 
> > If I added "misexpect" as a new `MD_prof`tag, then code in places like 
> > `extractProfTotalWeight(...)` in IR/Metadata.cpp would have to be updated. 
> > I'm not sure how pervasive that pattern is, but I did  want to avoid 
> > introducing systemic changes like that.
> > 
> > https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336
> That's not exactly what i was asking.
> I'm specifically talking about how `"misexpect"` and `"branch_weigths"` carry 
> basically the same data,
> are set basically in the same place always, with the difference that 
> `"misexpect"` also has the switch case index.
> This is both somewhat bloaty, and is prone to getting out of sync.
> 
> I have two questions:
> 1. Can `"misexpect"` metadata simply not be invented, but can existing 
> `LLVMContext::MD_misexpect` simply be used?
> 2. Can `"misexpect"` be changed to not store the weights, but a reference to 
> the `LLVMContext::MD_misexpect` metadata that will be emitted anyway?
> 
Sorry for the misunderstanding. 

I think what you are suggesting is to recreate the misexpect metadata from the 
MD_prof "branch_weights metadata. Is that right?

I think that may be possible, but it will add complexity. While the metadata 
approach is very straightforward. I think there is value in keeping this 
simple, and then iterating on it. 

Also, LowerExpectIntrinsic.cpp has a comment near the top suggesting that the 
LikelyBranchWeight and UnlikelyBranchWeights be hoisted to a common space. This 
would significantly shrink the size of the misexpect metadata if it were 
accessible directly. However, that is a change that should happen separately 
from this patch, and might require further discussion to find the correct 
shared space.

Ultimately, however, I do not see a way to completely remove some kind of 
metadata tag, as we only need to perform misexpect checks when the intrinsic 
was used -- which should be a small minority of cases. If there is another way, 
then I am happy to hear it, but again, maybe that should be another patch.





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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Missing tests for `__builtin_unpredictable()`.




Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+  unsigned Line, Column;
+  bool BadDebugInfo = false;
+  FullSourceLoc Loc =

paulkirth wrote:
> lebedev.ri wrote:
> > This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
> That should be in a separate patch, right?
Yeah, let's keep that for later.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:369
 void UnsupportedDiagHandler(const llvm::DiagnosticInfoUnsupported );
+void MisExpectDiagHandler(const llvm::DiagnosticInfoMisExpect );
 /// Specialized handlers for optimization remarks.

Comment missing



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3449
 
+  if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");

Err, this works? I'd expect `!` to be missing here..



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+  LLVMContext::MD_misexpect,
+  MDBuilder(CI->getContext())
+  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);

paulkirth wrote:
> lebedev.ri wrote:
> > Why can't `LLVMContext::MD_prof` be reused?
> It didn't seem appropriate to me, but that can be changed if it is the right 
> thing to do. 
> 
> However, I don't see code elsewhere that loops over `MD_prof` metadata 
> checking its tag. I see lots of examples that check if the tag matches, and 
> then exits if it doesn't match. 
> 
> If I added "misexpect" as a new `MD_prof`tag, then code in places like 
> `extractProfTotalWeight(...)` in IR/Metadata.cpp would have to be updated. 
> I'm not sure how pervasive that pattern is, but I did  want to avoid 
> introducing systemic changes like that.
> 
> https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336
That's not exactly what i was asking.
I'm specifically talking about how `"misexpect"` and `"branch_weigths"` carry 
basically the same data,
are set basically in the same place always, with the difference that 
`"misexpect"` also has the switch case index.
This is both somewhat bloaty, and is prone to getting out of sync.

I have two questions:
1. Can `"misexpect"` metadata simply not be invented, but can existing 
`LLVMContext::MD_misexpect` simply be used?
2. Can `"misexpect"` be changed to not store the weights, but a reference to 
the `LLVMContext::MD_misexpect` metadata that will be emitted anyway?




Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:141
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect

Here and elsewhere:
is `!prof !1` no longer present?



Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:292-293
+; CHECK: !3 = !{!"misexpect", i64 1, i64 2000, i64 1}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}

Should there be a FIXME, are some `misexpect` missing here?


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.

In D66324#1653198 , @paulkirth wrote:

> In D66324#1652364 , @lebedev.ri 
> wrote:
>
> > Trying to start reviewing this.
> >  The llvm side of the patch is self contained; clang patch should be split 
> > into a dependent review.
>
>
> Looking at this, is it necessary to split up the patch? That will loose a lot 
> of the previous comment/context, and I'd prefer to land this as a single 
> change.  Doing so has the benefit that if my patch needs to be reverted, it 
> can be done all together.  For example, if only the llvm patch is reverted, 
> then it will likely cause other problems and make triage needlessly painful.


I'd actually prefer to land this as a single change, it makes it easier to 
revert and look at the diff later.


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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218171.
paulkirth added a comment.

Fix comment for misexpect option


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218168.
paulkirth added a comment.

Remove extra include from CodeGenFunction.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

In D66324#1652364 , @lebedev.ri wrote:

> Trying to start reviewing this.
>  The llvm side of the patch is self contained; clang patch should be split 
> into a dependent review.


Looking at this, is it necessary to split up the patch? That will loose a lot 
of the previous comment/context, and I'd prefer to land this as a single 
change.  Doing so has the benefit that if my patch needs to be reverted, it can 
be done all together.  For example, if only the llvm patch is reverted, then it 
will likely cause other problems and make triage needlessly painful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218164.
paulkirth added a comment.

Address Review items

- minimize debug info in llvm tests
- sanitize paths in debug info
- use more complete checks in test  for LowerExpectIntrisic
- remove references to __builtin_expect from backend code
  - update test code for new diagnostic messages
- rename checkClangInstrumentation to checkFrontendInstrumentation
- revert whitespace removal in CodeGenFunction.cpp
- use more idomatic check when adding diagnostic to the backend


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch-stripped.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of the llvm.expect intrinsic: Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: 

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

[Paul Kirth via Phabricator via cfe-commits]

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

Thanks for the review.

I'm working on splitting up the patch now.

I should be able to address most of your comments, but I left some inline 
comments for clarification/discussion.




Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+  unsigned Line, Column;
+  bool BadDebugInfo = false;
+  FullSourceLoc Loc =

lebedev.ri wrote:
> This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
That should be in a separate patch, right?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+  LLVMContext::MD_misexpect,
+  MDBuilder(CI->getContext())
+  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);

lebedev.ri wrote:
> Why can't `LLVMContext::MD_prof` be reused?
It didn't seem appropriate to me, but that can be changed if it is the right 
thing to do. 

However, I don't see code elsewhere that loops over `MD_prof` metadata checking 
its tag. I see lots of examples that check if the tag matches, and then exits 
if it doesn't match. 

If I added "misexpect" as a new `MD_prof`tag, then code in places like 
`extractProfTotalWeight(...)` in IR/Metadata.cpp would have to be updated. I'm 
not sure how pervasive that pattern is, but I did  want to avoid introducing 
systemic changes like that.

https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:82
 
-  SI.setCondition(ArgValue);
   return true;

lebedev.ri wrote:
> Why do you need to move this line?
I did this so that the condition is available to check when performing 
validation, and the metadata won't be replaced by the likely/unlikely values 
until after I've finished the validation. Having the condition available in 
when performing the check provides for better diagnostics output.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into 
a dependent review.




Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+  unsigned Line, Column;
+  bool BadDebugInfo = false;
+  FullSourceLoc Loc =

This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:28-1361
 #include "clang/AST/StmtObjC.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "llvm/IR/DataLayout.h"

Dubious changes, drop



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3451-3454
+  const std::vector  = Res.getDiagnosticOpts().Warnings;
+  if (std::find(warnings.begin(), warnings.end(), "misexpect") !=
+  warnings.end())
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");

Here you are checking that `-Wmisexpect` is passed to clang?
This doesn't seem idiomatic. Was this copied from somewhere?
Normally this is `Diags.isIgnored(diag::warn_profile_data_misexpect, ???)`



Comment at: llvm/include/llvm/Transforms/Utils/MisExpect.h:1
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data 
===//
+//

`__builtin_expect()` is a clang-specific thing.
Can all instances of it in all comments be reworded to be more agnostic?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+  LLVMContext::MD_misexpect,
+  MDBuilder(CI->getContext())
+  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);

Why can't `LLVMContext::MD_prof` be reused?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:82
 
-  SI.setCondition(ArgValue);
   return true;

Why do you need to move this line?



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:84
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);
 

clang is not the only llvm frontend, this should not be so specific



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:30
+#include "llvm/Support/FormatVariadic.h"
+#include 
+#include 

This doesn't look correct.



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:33
+#include 
+#include 
+





Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:288-289
 
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}

`; CHECK: !1 = ???` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added a comment.

Sorry about that. Still LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218017.
paulkirth added a comment.

Actually address nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; DISABLED-NOT: remark: 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 218016.
paulkirth added a comment.

Address nits

- Add pointers to uses of auto
- Remove redundant cast
- Use consistent type for integers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

@leonardchan can you revise your review against the latest revision? It looks 
to me as though you might have reviewed one of the first diffs instead of what 
is the latest code. For example the files in clang-tools have not been part of 
this revision for some time. Also when I try to respond to the comment, it 
appears against the very first diff. I'm happy to address any of the nits that 
remain, but want to make sure that what you reviewed and given approval for is 
what will be submitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added inline comments.



Comment at: clang-tools-extra/clang-misexpect/ClangMisExpect.cpp:64
+break;
+  default:
+llvm_unreachable("Bad Profile Format given to clang-misexpect use one of "

Nit: The default and llvm_unreachable aren't needed here since you cover all 
cases already.



Comment at: llvm/lib/IR/MDBuilder.cpp:315
+   uint64_t UnlikleyWeight) {
+  auto IntType = Type::getInt64Ty(Context);
+  Metadata *Vals[] = {

nit: `auto *IntType`



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:55
+  Instruction *Ret = nullptr;
+  if (auto B = dyn_cast(I)) {
+Ret = dyn_cast(B->getCondition());

nit: `auto *B`



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:124
+  Weights.begin(), Weights.end(), (uint64_t)0, std::plus());
+  const int NumUnlikelyTargets = Weights.size() - 1;
+

nit: `const unsigned NumUnlikelyTargets`



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:129
+
+  double Percentage = (double)ProfileCount / (double)CaseTotal;
+

nit: `(double)ProfileCount / CaseTotal`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

Ok, thanks for the iteration on code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Looks good, will likely approve after these 2 questions.




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;
+

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.



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 }

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217528.
paulkirth added a comment.

Add comment to clarify choice of operands and reasoning about profiling metadata


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: warning: 

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

[Nick Desaulniers via Phabricator via cfe-commits]

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(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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-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(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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

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(MD->getOperand(0))) {
+  unsigned NOps = MD->getNumOperands();

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth marked 3 inline comments 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(MD->getOperand(0))) {
+  unsigned NOps = MD->getNumOperands();

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217494.
paulkirth added a comment.

Change check for minimum number of metadata operands.

Branch weights must have at least 3 elements:  metadata name, and a list of at 
least 2 branch weights


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217491.
paulkirth marked 2 inline comments as done.
paulkirth added a comment.

Remove namespace qualifier & insert check for number of arguments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled 

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

[Nick Desaulniers via Phabricator via cfe-commits]

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(MD->getOperand(0))) {
+  unsigned NOps = MD->getNumOperands();

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.



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:159
+}
+misexpect::verifyMisExpect(, RealWeights, I.getContext());
+  }

do you need the `misexpect::` qualifier here? I think your `using` statement 
above should make this one unnecessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1009
+public:
+  DiagnosticInfoMisExpect(const Function , const Twine ,
+  const DiagnosticLocation  = DiagnosticLocation(),

nickdesaulniers wrote:
> Is this constructor used anywhere in this patch?
No, sorry, I changed this interface and failed to remove the constructor.



Comment at: llvm/lib/IR/DiagnosticInfo.cpp:378
+ Inst->getDebugLoc()),
+  Msg(Msg) {}
+

nickdesaulniers wrote:
> Why is this constructor defined out of line, when the other is in the header? 
>  Seems like they should either both be in the header, or both be out of line, 
> with the goal being consistency.
I removed the other constructor, which was unused. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217479.
paulkirth added a comment.

Removes unused constructor, reformat code, and remove braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; DISABLED-NOT: warning: 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3451
 
+const std::vector  =
+  Res.getDiagnosticOpts().Warnings;

The indentation here looks funny. Can you please run `git-clang-format` and 
commit the changes on top?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3456
+Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
+  }
+

LLVM style is no `{}` for single statement bodies.



Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1009
+public:
+  DiagnosticInfoMisExpect(const Function , const Twine ,
+  const DiagnosticLocation  = DiagnosticLocation(),

Is this constructor used anywhere in this patch?



Comment at: llvm/lib/IR/DiagnosticInfo.cpp:378
+ Inst->getDebugLoc()),
+  Msg(Msg) {}
+

Why is this constructor defined out of line, when the other is in the header?  
Seems like they should either both be in the header, or both be out of line, 
with the goal being consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:123
+  const uint64_t CaseTotal = std::accumulate(
+  Weights.begin(), Weights.end(), (uint64_t)0, std::plus());
+  const int NumUnlikelyTargets = Weights.size() - 1;

nickdesaulniers wrote:
> s/(uint64_t)0/0ULL/ might be simpler, but I don't know if `long long == 
> uint64_t` on every host Clang can be built for...
I'm not sure if that is true, either. 

I think its safe to follow the convention of existing LLVM code doing more or 
less the same thing, though, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217285.
paulkirth marked an inline comment as done.
paulkirth added a comment.

Revert to use of auto when extracting metadata


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:108
+  // extract values from misexpect metadata
+  const ConstantInt *IndexCint =
+  mdconst::dyn_extract(MisExpectData->getOperand(1));

`const auto *IndexCInt`

`auto` should be used when the type is used in the rhs of an assignment such as 
in a template specifier IMO.  Ditto for below.



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:123
+  const uint64_t CaseTotal = std::accumulate(
+  Weights.begin(), Weights.end(), (uint64_t)0, std::plus());
+  const int NumUnlikelyTargets = Weights.size() - 1;

s/(uint64_t)0/0ULL/ might be simpler, but I don't know if `long long == 
uint64_t` on every host Clang can be built for...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:108
+  auto *L = 
mdconst::dyn_extract(MisExpectData->getOperand(2));
+  auto *U = 
mdconst::dyn_extract(MisExpectData->getOperand(3));
+

nickdesaulniers wrote:
> What's C, L, and U?
These are the constant integers for:
C = Index into the branch weight vector
L = likely branch weight
U = unlikely branch weight

The patch renames these to be a bit more transparent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 217282.
paulkirth marked 3 inline comments as done.
paulkirth added a comment.

Address Code Review

- Give better names to extracted constants
- Remove extraneous call to getValue()
- Make integers const
- Remove extra braces
- Move NOps into branch
- Use std::plus


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; BOTH-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; BOTH-DAG: remark: 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Cool, I look forward to checking for outdated `__builtin_expects` in the Linux 
kernel, if they exist!




Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:108
+  auto *L = 
mdconst::dyn_extract(MisExpectData->getOperand(2));
+  auto *U = 
mdconst::dyn_extract(MisExpectData->getOperand(3));
+

What's C, L, and U?



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:116
+  std::accumulate(Weights.begin(), Weights.end(), (uint64_t)0,
+  [](uint64_t W1, uint64_t W2) { return W1 + W2; });
+  int NumUnlikelyTargets = Weights.size() - 1;

can `std::plus` be used here?



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:122
+
+  double Percentage = ((double)ProfileCount / (double)CaseTotal);
+

are the extra parens necessary?



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:142
+// Operand 0 is a string tag "VP":
+if (MDString *Tag = cast(MD->getOperand(0))) {
+  if (NOps > 1 && Tag->getString().equals("branch_weights")) {

Do you need to check first that `NOps > 0` first? Otherwise you can likely move 
the creation of `NOps` below this conditional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a reviewer: nickdesaulniers.
paulkirth added a comment.

Add Nick Desaulniers as an additional reviewer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 216274.
paulkirth added a comment.
Herald added a subscriber: ormris.

Update LLVM based implementation

Adds LLVM based tests
Cleans up existing tests in clang
Adds new llvm flag -pgo-warn-misexpect
Adds llvm flag when -Wmisexpect enabled in frontend
Adds new debug output 
Fixes error when calculating & scaling branch weights
Improves fidelity of diagnostic outpus from SwithInst
Removes Remark output from MisExpecDiagHandler & uses ORE directly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-branch.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch-correct.proftext
  llvm/test/Transforms/PGOProfile/Inputs/misexpect-switch.proftext
  llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll
  llvm/test/Transforms/PGOProfile/misexpect-branch.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch-default.ll
  llvm/test/Transforms/PGOProfile/misexpect-switch.ll

Index: llvm/test/Transforms/PGOProfile/misexpect-switch.ll
===
--- /dev/null
+++ llvm/test/Transforms/PGOProfile/misexpect-switch.ll
@@ -0,0 +1,293 @@
+
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
+; RUN: llvm-profdata merge %S/Inputs/misexpect-switch-correct.proftext -o %t.c.profdata
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=REMARK
+; 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 --check-prefix=BOTH
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; New PM
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -S 2>&1 | FileCheck %s --check-prefix=WARNING
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=BOTH
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.c.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s --check-prefix=CORRECT
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.c.profdata -pgo-warn-misexpect -pass-remarks=misexpect -S 2>&1 | FileCheck %s --check-prefix=CORRECT
+
+; WARNING-DAG: warning: misexpect-switch.c:26:5: 0.00%
+; WARNING-NOT: remark: misexpect-switch.c:26:5: Potential performance regression from use of __builtin_expect(): Annotation was correct on 0.00% of profiled executions.
+
+; REMARK-NOT: warning: misexpect-switch.c:26:5: 0.00%
+; REMARK-DAG: remark: misexpect-switch.c:26:5: Potential performance regression from 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 215940.
paulkirth added a comment.

Fix missing context in prior diff


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-no-warning-without-flag.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -138,7 +138,7 @@
   %conv1 = sext i32 %conv to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv1, i64 0)
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect
   br i1 %tobool, label %if.then, label %if.end
 
@@ -165,7 +165,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !2
+; CHECK: !prof !4
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 1, label %sw.bb
@@ -194,7 +194,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !3
+; CHECK: !prof !5
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 2, label %sw.bb
@@ -278,7 +278,7 @@
   %t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
   %t8 = icmp ne i64 %t7, 0
   %t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
   ret i32 %t9
 }
 
@@ -286,6 +286,6 @@
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
Index: llvm/test/ThinLTO/X86/lazyload_metadata.ll
===
--- llvm/test/ThinLTO/X86/lazyload_metadata.ll
+++ llvm/test/ThinLTO/X86/lazyload_metadata.ll
@@ -10,13 +10,13 @@
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=LAZY
-; LAZY: 61 bitcode-reader  - Number of Metadata records loaded
+; LAZY: 63 bitcode-reader  - Number of Metadata records loaded
 ; LAZY: 2 bitcode-reader  - Number of MDStrings loaded
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -disable-ondemand-mds-loading -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=NOTLAZY
-; NOTLAZY: 70 bitcode-reader  - Number of Metadata records loaded
+; NOTLAZY: 72 bitcode-reader  - Number of Metadata records loaded
 ; NOTLAZY: 7 bitcode-reader  - Number of MDStrings loaded
 
 
Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -0,0 +1,125 @@
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This contains 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 215938.
paulkirth edited the summary of this revision.
paulkirth added a comment.

- Update CodeGenOptions to remove -fmisexpect
- Access the LLVMContext directly
- Add -line-tables-only for more accurate diagnostics

Fixes various issues with tests and cleans up some residual code from the 
original frontend implementation


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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/test/Profile/Inputs/misexpect-switch-default.proftext
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/lib/Transforms/Utils/MisExpect.cpp

Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- llvm/lib/Transforms/Utils/MisExpect.cpp
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -114,7 +114,7 @@
   mdconst::dyn_extract(MD->getOperand(i));
   RealWeights[i - 1] = Value->getZExtValue();
 }
-misexpect::verifyMisExpect(, RealWeights, I.getParent()->getParent()->getContext());
+misexpect::verifyMisExpect(, RealWeights, I.getContext());
   }
 }
   }
Index: clang/test/Profile/misexpect-switch.c
===
--- clang/test/Profile/misexpect-switch.c
+++ clang/test/Profile/misexpect-switch.c
@@ -1,7 +1,7 @@
 // Test that misexpect detects mis-annotated switch statements
 
 // RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 int sum(int *buff, int size);
 int random_sample(int *buff, int size);
Index: clang/test/Profile/misexpect-switch-only-default-case.c
===
--- clang/test/Profile/misexpect-switch-only-default-case.c
+++ clang/test/Profile/misexpect-switch-only-default-case.c
@@ -1,7 +1,7 @@
 // Test that misexpect emits no warning when there is only one switch case
 
 // RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default-only.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -fmisexpect
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 // expected-no-diagnostics
 int sum(int *buff, int size);
Index: clang/test/Profile/misexpect-switch-default.c
===
--- clang/test/Profile/misexpect-switch-default.c
+++ clang/test/Profile/misexpect-switch-default.c
@@ -1,7 +1,7 @@
 // Test that misexpect detects mis-annotated switch statements for default case
 
-// RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+// RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 int sum(int *buff, int size);
 int random_sample(int *buff, int size);
@@ -17,26 +17,24 @@
 int main() {
   init_arry();
   int val = 0;
-
-  int j, k;
-  for (j = 0; j < outer_loop; ++j) {
-for (k = 0; k < inner_loop; ++k) {
-  unsigned condition = rand() % 5;
-  switch (__builtin_expect(condition, 6)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}}
-  case 0:
-val += sum(arry, arry_size);
-break;
-  case 1:
-  case 2:
-  case 3:
-  case 4:
-val += random_sample(arry, arry_size);
-break;
-  default:
-__builtin_unreachable();
-  } // end switch
-}   // end inner_loop
-  } // end outer_loop
+  int j;
+  for (j = 0; j < outer_loop * inner_loop; ++j) {
+unsigned condition = rand() % 5;
+switch (__builtin_expect(condition, 6)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}}
+case 0:
+  val += sum(arry, arry_size);
+  break;
+case 1:
+case 2:
+case 3:
+  break;
+case 4:
+  val += random_sample(arry, arry_size);
+  break;
+default:
+  __builtin_unreachable();
+} // end switch
+  }   // end outer_loop
 
   return 0;
 }
Index: clang/test/Profile/misexpect-branch.c

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 215734.
paulkirth added a comment.

Remove frontend components of clang-misexpect in favor of backend 
implementations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-no-warning-without-flag.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -138,7 +138,7 @@
   %conv1 = sext i32 %conv to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv1, i64 0)
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect
   br i1 %tobool, label %if.then, label %if.end
 
@@ -165,7 +165,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !2
+; CHECK: !prof !4
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 1, label %sw.bb
@@ -194,7 +194,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !3
+; CHECK: !prof !5
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 2, label %sw.bb
@@ -278,7 +278,7 @@
   %t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
   %t8 = icmp ne i64 %t7, 0
   %t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
   ret i32 %t9
 }
 
@@ -286,6 +286,6 @@
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
Index: llvm/test/ThinLTO/X86/lazyload_metadata.ll
===
--- llvm/test/ThinLTO/X86/lazyload_metadata.ll
+++ llvm/test/ThinLTO/X86/lazyload_metadata.ll
@@ -10,13 +10,13 @@
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=LAZY
-; LAZY: 61 bitcode-reader  - Number of Metadata records loaded
+; LAZY: 63 bitcode-reader  - Number of Metadata records loaded
 ; LAZY: 2 bitcode-reader  - Number of MDStrings loaded
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -disable-ondemand-mds-loading -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=NOTLAZY
-; NOTLAZY: 70 bitcode-reader  - Number of Metadata records loaded
+; NOTLAZY: 72 bitcode-reader  - Number of Metadata records loaded
 ; NOTLAZY: 7 bitcode-reader  - Number of MDStrings loaded
 
 
Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -0,0 +1,125 @@
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.



> So it is possible to handle this completely in the backed, but the diagnostic 
> output is not fantastic when using clang based instrumentation. In 
> particular, we would need to emit the diagnostic in LowerExpectIntrisic.cpp 
> by checking if the branch weight metadata already exists, and processing it 
> there before it is overwritten. For some reason emitting the diagnostic at 
> that point will not also emit the helpful source listing if there is a macro, 
> even though a FullSourceLoc is available. This isn't an issue for either the 
> handling in SampleProfile.cpp or in PGOInstrumentation.cpp.

Actually, I made a mistake evaluating the diagnostic output. I redid my test 
for handling everything the Clang based instrumentation in 
LowerExpectIntrinsic.cpp and the output now looks as expected, so I must have 
had a logic error somewhere.

I think this can be handled 100% in the backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 215646.
paulkirth added a comment.

Remove reference to clang-misexpect from CMakeLists.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/MisExpect.cpp
  clang/lib/CodeGen/MisExpect.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-no-warning-without-flag.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -138,7 +138,7 @@
   %conv1 = sext i32 %conv to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv1, i64 0)
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect
   br i1 %tobool, label %if.then, label %if.end
 
@@ -165,7 +165,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !2
+; CHECK: !prof !4
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 1, label %sw.bb
@@ -194,7 +194,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !3
+; CHECK: !prof !5
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 2, label %sw.bb
@@ -278,7 +278,7 @@
   %t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
   %t8 = icmp ne i64 %t7, 0
   %t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
   ret i32 %t9
 }
 
@@ -286,6 +286,6 @@
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
Index: llvm/test/ThinLTO/X86/lazyload_metadata.ll
===
--- llvm/test/ThinLTO/X86/lazyload_metadata.ll
+++ llvm/test/ThinLTO/X86/lazyload_metadata.ll
@@ -10,13 +10,13 @@
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=LAZY
-; LAZY: 61 bitcode-reader  - Number of Metadata records loaded
+; LAZY: 63 bitcode-reader  - Number of Metadata records loaded
 ; LAZY: 2 bitcode-reader  - Number of MDStrings loaded
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -disable-ondemand-mds-loading -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=NOTLAZY
-; NOTLAZY: 70 bitcode-reader  - Number of Metadata records loaded
+; NOTLAZY: 72 bitcode-reader  - Number of Metadata records loaded
 ; NOTLAZY: 7 bitcode-reader  - Number of MDStrings loaded
 
 
Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -0,0 +1,105 @@
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data ===//
+//
+// Part 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth updated this revision to Diff 215643.
paulkirth edited the summary of this revision.
paulkirth added a comment.
Herald added subscribers: dexonsmith, steven_wu, mehdi_amini.

Removes standalone clang-misexpect from revision


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324

Files:
  clang-tools-extra/CMakeLists.txt
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/MisExpect.cpp
  clang/lib/CodeGen/MisExpect.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-no-warning-without-flag.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/ThinLTO/X86/lazyload_metadata.ll
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -138,7 +138,7 @@
   %conv1 = sext i32 %conv to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv1, i64 0)
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect
   br i1 %tobool, label %if.then, label %if.end
 
@@ -165,7 +165,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !2
+; CHECK: !prof !4
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 1, label %sw.bb
@@ -194,7 +194,7 @@
   %tmp = load i32, i32* %x.addr, align 4
   %conv = sext i32 %tmp to i64
   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !3
+; CHECK: !prof !5
 ; CHECK-NOT: @llvm.expect
   switch i64 %expval, label %sw.epilog [
 i64 2, label %sw.bb
@@ -278,7 +278,7 @@
   %t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
   %t8 = icmp ne i64 %t7, 0
   %t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
   ret i32 %t9
 }
 
@@ -286,6 +286,6 @@
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
Index: llvm/test/ThinLTO/X86/lazyload_metadata.ll
===
--- llvm/test/ThinLTO/X86/lazyload_metadata.ll
+++ llvm/test/ThinLTO/X86/lazyload_metadata.ll
@@ -10,13 +10,13 @@
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=LAZY
-; LAZY: 61 bitcode-reader  - Number of Metadata records loaded
+; LAZY: 63 bitcode-reader  - Number of Metadata records loaded
 ; LAZY: 2 bitcode-reader  - Number of MDStrings loaded
 
 ; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc \
 ; RUN:  -o /dev/null -disable-ondemand-mds-loading -stats \
 ; RUN:  2>&1 | FileCheck %s -check-prefix=NOTLAZY
-; NOTLAZY: 70 bitcode-reader  - Number of Metadata records loaded
+; NOTLAZY: 72 bitcode-reader  - Number of Metadata records loaded
 ; NOTLAZY: 7 bitcode-reader  - Number of MDStrings loaded
 
 
Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- /dev/null
+++ 

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

In D66324#1632653 , @lebedev.ri wrote:

> This is marked as child revision of D65300  
> but it seems like they both add
>  the same logic, just into different components, D65300 
>  to clang, this to llvm.
>  Is this duplication or are the diffs concurrent?


So this change extends and revises what is in D65300 
.  The diffs are somewhat concurrent. For 
example the backend changes rely on the frontend warning being available. i.e. 
diag::warn_profile_data_misexpect.  But yes the logic is mostly the same.

I'm also not terribly familiar with Phabricator, so maybe I should have 
uploaded a diff between my two revisions, rather than between the my changes 
and top of tree?

> Can D65300  be re-implemented to just use 
> this llvm code?

So it is possible to handle this completely in the backed, but the diagnostic 
output is not fantastic when using clang based instrumentation. In particular, 
we would need to emit the diagnostic in LowerExpectIntrisic.cpp by checking if 
the branch weight metadata already exists, and processing it there before it is 
overwritten. For some reason emitting the diagnostic at that point will not 
also emit the helpful source listing if there is a macro, even though a 
FullSourceLoc is available. This isn't an issue for either the handling in 
SampleProfile.cpp or in PGOInstrumentation.cpp.

In the end, I decided that it actually made sense for clang to handle the clang 
based instrumentation, and llvm to handle the IR/Sampling instrumentation.

> I'm also thinking the clang-misexpect *tool* should be a separate change,
>  it's misleading to have it here as it gives impression that it **has** to be 
> used,
>  which i'm not sure is so?

Sure, I can update this so that the standalone tool is in a separate change. 
The thought was to consolidate all the related changes needed for the 
standalone tool, but I see your point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added reviewers: lebedev.ri, vsk.
lebedev.ri added a comment.

This is marked as child revision of D65300  
but it seems like they both add
the same logic, just into different components, D65300 
 to clang, this to llvm.
Is this duplication or are the diffs concurrent?
Can D65300  be re-implemented to just use this 
llvm code?

I'm also thinking the clang-misexpect *tool* should be a separate change,
it's misleading to have it here as it gives impression that it **has** to be 
used,
which i'm not sure is so?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth created this revision.
paulkirth added reviewers: phosek, leonardchan, jakehehrlich, mcgrathr.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya, mgorny.
Herald added projects: clang, LLVM.
paulkirth added a parent revision: D65300: [clang] [CodeGen] clang-misexpect 
prototype for compiler warnings.

This patch contains a prototype of the basic functionality of clang-misexpect 
in the PGO pipeline. clang-misexpect is a proposed clang-tool that can report 
potentially incorrect usage of __builtin_expect() by comparing the developer's 
annotation against a collected PGO profile. A more detailed proposal and 
discussion appears on the CFE-dev mailing list 
(http://lists.llvm.org/pipermail/cfe-dev/2019-July/062971.html) and a prototype 
of the frontend changes appear here on 
phabricator(https://reviews.llvm.org/D65300)

We extend the work in https://reviews.llvm.org/D65300 by adding support for IR 
and sampling based profiles to the backend. We also supply a standalone 
clang-misexpect tool built on top of libTooling. The standalone tool uses a 
compile commands database and profiling data to issue the warnings enabled by 
the new checks in Clang and LLVM.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66324

Files:
  clang-tools-extra/CMakeLists.txt
  clang-tools-extra/clang-misexpect/CMakeLists.txt
  clang-tools-extra/clang-misexpect/ClangMisExpect.cpp
  clang-tools-extra/clang-misexpect/ClangMisExpect.h
  clang-tools-extra/clang-misexpect/tool/CMakeLists.txt
  clang-tools-extra/clang-misexpect/tool/ClangMisExpectMain.cpp
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/MisExpect.cpp
  clang/lib/CodeGen/MisExpect.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext
  clang/test/Profile/Inputs/misexpect-branch.proftext
  clang/test/Profile/Inputs/misexpect-switch-default-only.proftext
  clang/test/Profile/Inputs/misexpect-switch.proftext
  clang/test/Profile/misexpect-branch-cold.c
  clang/test/Profile/misexpect-branch-nonconst-expected-val.c
  clang/test/Profile/misexpect-branch.c
  clang/test/Profile/misexpect-no-warning-without-flag.c
  clang/test/Profile/misexpect-switch-default.c
  clang/test/Profile/misexpect-switch-nonconst.c
  clang/test/Profile/misexpect-switch-only-default-case.c
  clang/test/Profile/misexpect-switch.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/IR/FixedMetadataKinds.def
  llvm/include/llvm/IR/MDBuilder.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/DiagnosticInfo.cpp
  llvm/lib/IR/MDBuilder.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/MisExpect.cpp

Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -0,0 +1,105 @@
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This contains code to emit warnings for potentially incorrect usage of
+// __builtin_expect(). This utility extracts the threshold values from metadata
+// associated with the instrumented Branch or Switch. The threshold values are
+// then used to determin if a warning would be emmited.
+//
+// MisExpect metadata is generated when llvm.expect intrinsics are lowered see
+// LowerExpectIntrinsic.cpp
+//
+//===--===//
+
+#include "llvm/Transforms/Utils/MisExpect.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/Support/BranchProbability.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/FormatVariadic.h"
+#include 
+#include 
+
+#define DEBUG_TYPE "misexpect"
+
+namespace {
+
+llvm::Instruction *getOprndOrInst(llvm::Instruction *I) {
+  llvm::Instruction *Ret = nullptr;
+  if (auto B = llvm::dyn_cast(I)) {
+Ret = llvm::dyn_cast(B->getCondition());
+  } else if (auto S = llvm::dyn_cast(I)) {
+Ret = llvm::dyn_cast(S->getCondition());
+  }
+  return Ret ? Ret : I;
+}
+
+void