[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi edited 
https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi updated 
https://github.com/llvm/llvm-project/pull/86609


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

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

ilovepi wrote:

@MatzeB Thanks for all the good suggestions. I'm just getting back form 
EuroLLVM + a vacation, but I'll try to set aside some time this afternoon to 
update the PR.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

@@ -5196,7 +5198,11 @@ void SwitchInstProfUpdateWrapper::init() {
   if (!ProfileData)
 return;
 
-  if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) {
+  // FIXME: This check belongs in ProfDataUtils. Its almost equivalent to
+  // getValidBranchWeightMDNode(), but the need to use llvm_unreachable
+  // makes them slightly different.
+  if (ProfileData->getNumOperands() !=
+  SI.getNumSuccessors() + getBranchWeightOffset(ProfileData)) {

ilovepi wrote:

Another good suggestion. Thank you.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

@@ -1210,12 +1210,22 @@ Instruction *Instruction::cloneImpl() const {
 
 void Instruction::swapProfMetadata() {
   MDNode *ProfileData = getBranchWeightMDNode(*this);
-  if (!ProfileData || ProfileData->getNumOperands() != 3)
+  if (!isBranchWeightMD(ProfileData))
 return;
 
-  // The first operand is the name. Fetch them backwards and build a new one.
-  Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2),
- ProfileData->getOperand(1)};
+  SmallVector Ops;
+  unsigned int FirstIdx = getBranchWeightOffset(ProfileData);
+  unsigned int SecondIdx = FirstIdx + 1;
+  // If there are more weights past the second, we can't swap them
+  if (ProfileData->getNumOperands() > SecondIdx + 1)
+return;
+  Ops.push_back(ProfileData->getOperand(0));
+  if (hasExpectedProvenance(ProfileData)) {
+Ops.push_back(ProfileData->getOperand(1));
+  }

ilovepi wrote:

That's a good suggestion. I'll update the patch to reflect.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

@@ -55,6 +55,20 @@ MDNode *getBranchWeightMDNode(const Instruction );
 /// Nullptr otherwise.
 MDNode *getValidBranchWeightMDNode(const Instruction );
 
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const Instruction );
+
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const MDNode *ProfileData);
+
+/// Return the offset to the first branch weight data
+unsigned getBranchWeightOffset(const Instruction );

ilovepi wrote:

That's a fair point. IIRC I needed this someplace, but maybe I'm 
mis-remembering and that's a moot point. I'll look at my follow up patches to 
ensure it isn't required.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

@@ -55,6 +55,20 @@ MDNode *getBranchWeightMDNode(const Instruction );
 /// Nullptr otherwise.
 MDNode *getValidBranchWeightMDNode(const Instruction );
 
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const Instruction );
+
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const MDNode *ProfileData);

ilovepi wrote:

Fair points ... maybe we should consider that refactoring anyway? I can file a 
Github issue to track it, and try to set aside some time over the next couple 
of weeks to look into it.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

https://github.com/MatzeB edited https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

https://github.com/MatzeB edited https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -4756,8 +4757,10 @@ void Verifier::visitProfMetadata(Instruction , MDNode 
*MD) {
 
   // Check consistency of !prof branch_weights metadata.
   if (ProfName.equals("branch_weights")) {
+unsigned int Offset = getBranchWeightOffset(I);
 if (isa()) {
-  Check(MD->getNumOperands() == 2 || MD->getNumOperands() == 3,
+  Check(MD->getNumOperands() == (1 + Offset) ||
+MD->getNumOperands() == (2 + Offset),

MatzeB wrote:

More opportunities for a possible `getNumBranchWeights(...)` API...

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -55,6 +55,20 @@ MDNode *getBranchWeightMDNode(const Instruction );
 /// Nullptr otherwise.
 MDNode *getValidBranchWeightMDNode(const Instruction );
 
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const Instruction );

MatzeB wrote:

Hmm maybe this should somehow have the name "Weight" in the name, to not 
introduce confusion with alias-analysis things (at least I immediately think 
"alias analysis" when I see the word  "provenance") or find different term than 
"provenance"?

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -55,6 +55,20 @@ MDNode *getBranchWeightMDNode(const Instruction );
 /// Nullptr otherwise.
 MDNode *getValidBranchWeightMDNode(const Instruction );
 
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const Instruction );
+
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const MDNode *ProfileData);

MatzeB wrote:

FWIW: I usually prefer references like `const MDNode ` to indicate 
that an argument mustn't be `nullptr`. Though admittedly that remark comes too 
late given we already have other `const MDNode *` APIs in this header 
consistency is also worth something... So I guess I'm fine either way...

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -1210,12 +1210,22 @@ Instruction *Instruction::cloneImpl() const {
 
 void Instruction::swapProfMetadata() {
   MDNode *ProfileData = getBranchWeightMDNode(*this);
-  if (!ProfileData || ProfileData->getNumOperands() != 3)
+  if (!isBranchWeightMD(ProfileData))
 return;
 
-  // The first operand is the name. Fetch them backwards and build a new one.
-  Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2),
- ProfileData->getOperand(1)};
+  SmallVector Ops;
+  unsigned int FirstIdx = getBranchWeightOffset(ProfileData);
+  unsigned int SecondIdx = FirstIdx + 1;
+  // If there are more weights past the second, we can't swap them
+  if (ProfileData->getNumOperands() > SecondIdx + 1)
+return;
+  Ops.push_back(ProfileData->getOperand(0));
+  if (hasExpectedProvenance(ProfileData)) {
+Ops.push_back(ProfileData->getOperand(1));
+  }

MatzeB wrote:

Maybe this (I just have a feeling that leaving it more generic may help in case 
new sources are added one day):

```suggestion
  for (unsigned I = 0; I < FirstIdx; I++) {
 Ops.push_back(ProfileData->getOperand(I));
 }
```

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -1210,12 +1210,22 @@ Instruction *Instruction::cloneImpl() const {
 
 void Instruction::swapProfMetadata() {
   MDNode *ProfileData = getBranchWeightMDNode(*this);
-  if (!ProfileData || ProfileData->getNumOperands() != 3)
+  if (!isBranchWeightMD(ProfileData))
 return;
 
-  // The first operand is the name. Fetch them backwards and build a new one.
-  Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2),
- ProfileData->getOperand(1)};
+  SmallVector Ops;
+  unsigned int FirstIdx = getBranchWeightOffset(ProfileData);

MatzeB wrote:

Not a strong opinion, but I think most people just write `unsigned` in LLVM 
codebase...
```suggestion
  unsigned FirstIdx = getBranchWeightOffset(ProfileData);
```

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -5196,7 +5198,11 @@ void SwitchInstProfUpdateWrapper::init() {
   if (!ProfileData)
 return;
 
-  if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) {
+  // FIXME: This check belongs in ProfDataUtils. Its almost equivalent to
+  // getValidBranchWeightMDNode(), but the need to use llvm_unreachable
+  // makes them slightly different.
+  if (ProfileData->getNumOperands() !=
+  SI.getNumSuccessors() + getBranchWeightOffset(ProfileData)) {

MatzeB wrote:

This seems simple enough to do something about it? Could for example add a 
`getNumBranchWeights()` API so this can become 
`getNumBranchWeights(ProfileData) != SI.getNumSuccessors()`?

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -106,6 +104,30 @@ bool hasValidBranchWeightMD(const Instruction ) {
   return getValidBranchWeightMDNode(I);
 }
 
+bool hasExpectedProvenance(const Instruction ) {
+  auto *ProfileData = I.getMetadata(LLVMContext::MD_prof);
+  return hasExpectedProvenance(ProfileData);
+}
+
+bool hasExpectedProvenance(const MDNode *ProfileData) {
+  if (!isBranchWeightMD(ProfileData))
+return false;
+
+  auto *ProfDataName = dyn_cast(ProfileData->getOperand(1));
+  if (!ProfDataName)
+return false;
+  return ProfDataName->getString().equals("expected");
+}
+
+unsigned getBranchWeightOffset(const Instruction ) {
+  auto *ProfileData = I.getMetadata(LLVMContext::MD_prof);
+  return getBranchWeightOffset(ProfileData);
+}
+
+unsigned getBranchWeightOffset(const MDNode *ProfileData) {
+  return hasExpectedProvenance(ProfileData) ? 2 : 1;
+}

MatzeB wrote:

What about a `hasBranchWeightProvenance()` API instead that just checks whether 
there is a string? That way you would get the same effect today but can skip 
the string comparison (and maybe get better behavior if there is a string that 
isn't actually "expected")

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

@@ -55,6 +55,20 @@ MDNode *getBranchWeightMDNode(const Instruction );
 /// Nullptr otherwise.
 MDNode *getValidBranchWeightMDNode(const Instruction );
 
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const Instruction );
+
+/// Check if Branch Weight Metadata has an "expected" field from an 
llvm.expect*
+/// intrinsic
+bool hasExpectedProvenance(const MDNode *ProfileData);
+
+/// Return the offset to the first branch weight data
+unsigned getBranchWeightOffset(const Instruction );

MatzeB wrote:

Is this API really helpful? Don't you have to get your hands on an `MDNode` 
anyway to do something useful with that offset?

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

https://github.com/MatzeB commented:

I guess this is deliberately designed around a `bool IsExpected` rather than a 
more generic API. I guess I'm fine with it given the current patch, though I 
would bet it's just a question of time for someone to add more sources now... 
(so gotta make sure to turn the API into a generic one when there is more 
sources coming around)

Added a bunch of nitpicks, naming discussions etc. Though all-in-all this looks 
good to me.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Matthias Braun via llvm-branch-commits]

https://github.com/MatzeB edited https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

@@ -1210,12 +1210,22 @@ Instruction *Instruction::cloneImpl() const {
 
 void Instruction::swapProfMetadata() {
   MDNode *ProfileData = getBranchWeightMDNode(*this);
-  if (!ProfileData || ProfileData->getNumOperands() != 3)
+  if (!isBranchWeightMD(ProfileData))

ilovepi wrote:

Oh, that's a good point. In my head these were all tied together w/ the change 
to the metadata layout, but maybe I can restructure ProfdataUtils first, and 
then update the surrounding code, and after that's done introduce the metadata 
changes. Thanks for the suggestion. I'll take a pass at that soon.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[David Li via llvm-branch-commits]

@@ -1210,12 +1210,22 @@ Instruction *Instruction::cloneImpl() const {
 
 void Instruction::swapProfMetadata() {
   MDNode *ProfileData = getBranchWeightMDNode(*this);
-  if (!ProfileData || ProfileData->getNumOperands() != 3)
+  if (!isBranchWeightMD(ProfileData))

david-xl wrote:

Extract this and other similar refactoring change into a different patch?

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi edited 
https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

ilovepi wrote:

ping.

https://github.com/llvm/llvm-project/pull/86609
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[via llvm-branch-commits]

llvmbot wrote:




@llvm/pr-subscribers-pgo

Author: Paul Kirth (ilovepi)


Changes

This patch implements the changes to LLVM IR discussed in
https://discourse.llvm.org/t/rfc-update-branch-weights-metadata-to-allow-tracking-branch-weight-origins/75032

In this patch, we add an optional field to MD_prof meatdata nodes for
branch weights, which can be used to distinguish weights added from
`llvm.expect*` intrinsics from those added via other methods, e.g.
from profiles or inserted by the compiler.

One of the major motivations, is for use with MisExpect diagnostics,
which need to know if branch_weight metadata originates from an
llvm.expect intrinsic. Without that information, we end up checking
branch weights multiple times in the case if ThinLTO + SampleProfiling,
leading to some inaccuracy in how we report MisExpect related
diagnostics to users.

Since we change the format of MD_prof metadata in a fundamental way, we
need to update code handling branch weights in a number of places.

We also update the lang ref for branch weights to reflect the change.


---

Patch is 37.48 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/86609.diff


28 Files Affected:

- (modified) clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp 
(+2-2) 
- (modified) llvm/docs/BranchWeightMetadata.rst (+7) 
- (modified) llvm/include/llvm/IR/MDBuilder.h (+9-2) 
- (modified) llvm/include/llvm/IR/ProfDataUtils.h (+18-1) 
- (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+2-1) 
- (modified) llvm/lib/IR/Instruction.cpp (+14-4) 
- (modified) llvm/lib/IR/Instructions.cpp (+10-4) 
- (modified) llvm/lib/IR/MDBuilder.cpp (+9-5) 
- (modified) llvm/lib/IR/ProfDataUtils.cpp (+34-14) 
- (modified) llvm/lib/IR/Verifier.cpp (+6-3) 
- (modified) llvm/lib/Transforms/IPO/SampleProfile.cpp (+4-3) 
- (modified) llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp 
(+1-1) 
- (modified) llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp 
(+2-1) 
- (modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+2-2) 
- (modified) llvm/lib/Transforms/Scalar/JumpThreading.cpp (+2-2) 
- (modified) llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp (+9-7) 
- (modified) llvm/lib/Transforms/Utils/Local.cpp (+1-1) 
- (modified) llvm/lib/Transforms/Utils/LoopPeel.cpp (+2-2) 
- (modified) llvm/lib/Transforms/Utils/LoopRotationUtils.cpp (+2-2) 
- (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+10-10) 
- (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+6-6) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/basic.ll (+4-4) 
- (modified) 
llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll (+4-4) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll 
(+2-3) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll (+2-2) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll (+2-2) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll (+1-1) 
- (modified) llvm/test/Transforms/LowerExpectIntrinsic/phi_unexpect.ll (+2-2) 


``diff
diff --git a/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp 
b/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
index fb236aeb982e01..81d93343565209 100644
--- a/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
+++ b/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
@@ -221,5 +221,5 @@ void tu2(int ) {
   }
 }
 
-// CHECK: [[BW_LIKELY]] = !{!"branch_weights", i32 2000, i32 1}
-// CHECK: [[BW_UNLIKELY]] = !{!"branch_weights", i32 1, i32 2000}
+// CHECK: [[BW_LIKELY]] = !{!"branch_weights", !"expected", i32 2000, i32 1}
+// CHECK: [[BW_UNLIKELY]] = !{!"branch_weights", !"expected", i32 1, i32 2000}
diff --git a/llvm/docs/BranchWeightMetadata.rst 
b/llvm/docs/BranchWeightMetadata.rst
index 522f37cdad4fc1..62204753e29b06 100644
--- a/llvm/docs/BranchWeightMetadata.rst
+++ b/llvm/docs/BranchWeightMetadata.rst
@@ -28,11 +28,14 @@ Supported Instructions
 
 Metadata is only assigned to the conditional branches. There are two extra
 operands for the true and the false branch.
+We optionally track if the metadata was added by ``__builtin_expect`` or
+``__builtin_expect_with_probability`` with an optional field ``!"expected"``.
 
 .. code-block:: none
 
   !0 = !{
 !"branch_weights",
+[ !"expected", ]
 i32 ,
 i32 
   }
@@ -47,6 +50,7 @@ is always case #0).
 
   !0 = !{
 !"branch_weights",
+[ !"expected", ]
 i32 
 [ , i32  ... ]
   }
@@ -60,6 +64,7 @@ Branch weights are assigned to every destination.
 
   !0 = !{
 !"branch_weights",
+[ !"expected", ]
 i32 
 [ , i32  ... ]
   }
@@ -75,6 +80,7 @@ block and entry counts which may not be accurate with 
sampling.
 
   !0 = !{
 !"branch_weights",
+[ !"expected", ]
 i32 
   }
 
@@ -95,6 +101,7 @@ is used.
 
   !0 = !{
 !"branch_weights",
+[ !"expected", ]
 i32 
 [ , i32  ]
   }
diff --git 

[llvm-branch-commits] [llvm][IR] Extend BranchWeightMetadata to track provenance of weights (PR #86609)

[Paul Kirth via llvm-branch-commits]

https://github.com/ilovepi created 
https://github.com/llvm/llvm-project/pull/86609

This patch implements the changes to LLVM IR discussed in
https://discourse.llvm.org/t/rfc-update-branch-weights-metadata-to-allow-tracking-branch-weight-origins/75032

In this patch, we add an optional field to MD_prof meatdata nodes for
branch weights, which can be used to distinguish weights added from
`llvm.expect*` intrinsics from those added via other methods, e.g.
from profiles or inserted by the compiler.

One of the major motivations, is for use with MisExpect diagnostics,
which need to know if branch_weight metadata originates from an
llvm.expect intrinsic. Without that information, we end up checking
branch weights multiple times in the case if ThinLTO + SampleProfiling,
leading to some inaccuracy in how we report MisExpect related
diagnostics to users.

Since we change the format of MD_prof metadata in a fundamental way, we
need to update code handling branch weights in a number of places.

We also update the lang ref for branch weights to reflect the change.



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