Page MenuHomePhabricator

Extend InvokeInst !prof branch_weights metadata to unwind branches
ClosedPublic

Authored by yrouban on May 27 2020, 3:10 AM.

Details

Summary

Allow InvokeInst to have the second optional prof branch weight for its unwind branch.
InvokeInst is a terminator with two successors. It might have its unwind branch taken many times. If so the BranchProbabilityInfo unwind branch heuristic can be inaccurate. This patch allows a higher accuracy calculated with both branch weights set.

Changes:

  • A new section about InvokeInst is added to the BranchWeightMetadata page. It states the old information that missed in the doc and adds new about the second branch weight.
  • Verifier is changed to allow either 1 or 2 branch weights for InvokeInst.
  • A new test is written for BranchProbabilityInfo to demonstrate the main improvement of the simple fix in calcMetadataWeights().
  • Several new testcases are created for Inliner. Those check that both weights are accounted for invoke instruction weight calculation.

Diff Detail

Event Timeline

yrouban created this revision.May 27 2020, 3:10 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
yrouban marked an inline comment as done.May 27 2020, 11:14 PM
yrouban added inline comments.
llvm/lib/IR/Instruction.cpp
779 ↗(On Diff #266466)

return true;

yrouban updated this revision to Diff 266756.May 27 2020, 11:49 PM
yrouban added a reviewer: yamauchi.

fixed return statement in Instruction::copyProfWeights().

davidxl added inline comments.May 28 2020, 9:35 AM
llvm/lib/IR/Instruction.cpp
760 ↗(On Diff #266756)

naming nit: copyProfData or copyProfMetadata because it copies branchweights and VP data

yrouban updated this revision to Diff 267150.May 29 2020, 2:42 AM
yrouban marked an inline comment as done.
yrouban edited the summary of this revision. (Show Details)

Renamed method copyProfWeighs() to copyProfMetadata() and improved its description.

davidxl added a reviewer: xur.May 29 2020, 9:21 AM
yamauchi added inline comments.May 29 2020, 10:00 AM
llvm/include/llvm/IR/Instruction.h
337 ↗(On Diff #267150)

Can this function move to the CallBase class as it seems it's already specific to CallBase and all the existing call sites call on CallBase.

llvm/lib/IR/Verifier.cpp
4197

"Wrong number of InvokeInst branch_weights operands" to be clearer?

xur added a comment.May 29 2020, 11:17 AM

Thanks for the fix. We also noticed that the branch weight for invoke instructions were not set properly, even thought we had the profile information in PGO.
I have a check that disables setting the branch weight metadata for IRPGO. Once this patch is in. I can remove that check. Of you can integrate that change into this patch.
It's in PGOUseFunc::setBranchWeights(). Just white-list InvokeInst.

reames added a subscriber: reames.May 29 2020, 11:47 AM

Mostly minor comments on style to help drive review forward. This is a drive by comment, don't wait on me please.

I generally think this patch is a really good idea. Longer term, I think we should extend this to calls as well. There's nothing preventing a call from being hot throw w/o a local exception handler and the current static heuristic doesn't consider that case. I specifically do not want this rolled into the current patch. That would be scope creep. :)

llvm/docs/BranchWeightMetadata.rst
98

I'd suggest calling this INVOKE_NORMAL_WEIGHT.

llvm/lib/IR/Instruction.cpp
781 ↗(On Diff #267150)

Why not just have the argument be of CallBase type then?

791 ↗(On Diff #267150)

Note sure you need these validation checks here since the source can be presumed valid.

llvm/lib/IR/Verifier.cpp
4175–4176

Please rearrange this code as:
if (is invoke) {

do invoke check;

} else {

do current code block;

}

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
340 ↗(On Diff #267150)

It looks like copyProfMetadata could be pulled out into a separate NFC. Please do so.

davidxl accepted this revision.May 31 2020, 2:17 PM

lgtm with Philip and Rong's comments addressed.

This revision is now accepted and ready to land.May 31 2020, 2:17 PM
yrouban marked 6 inline comments as done.Jun 2 2020, 2:29 AM
In D80618#2063604, @xur wrote:

Thanks for the fix. We also noticed that the branch weight for invoke instructions were not set properly, even thought we had the profile information in PGO.
I have a check that disables setting the branch weight metadata for IRPGO. Once this patch is in. I can remove that check. Of you can integrate that change into this patch.
It's in PGOUseFunc::setBranchWeights(). Just white-list InvokeInst.

done

llvm/lib/IR/Instruction.cpp
791 ↗(On Diff #267150)

With this comment and having in mind the comments about type of this and SrcCB I decided to get rid of the method at all and just call NewCB->copyMetadata(OldCB, {LLVMContext::MD_prof}).

yrouban updated this revision to Diff 267825.Jun 2 2020, 2:34 AM
yrouban marked an inline comment as done.
yrouban edited the summary of this revision. (Show Details)

Addressed all comments.
Extracted setProfWeights() related changes to D80987.
I will land this patch once D80987 is reviewed and landed.

yrouban updated this revision to Diff 268060.Jun 2 2020, 10:31 PM

no changes but added wider context for ease of review

This revision was automatically updated to reflect the committed changes.
yrouban marked an inline comment as done.

Hi @yrouban
this commit breaks the following testcase. If you agree this is a regression, then can you please revert your commit until this is addressed.

; RUN: opt -simplifycfg -S %s
;
target datalayout = "e-m:e-i64:64-n32:64"
target triple = "powerpc64le-unknown-linux-gnu"

%cM = type { i32 }
%cE = type { i32 }

@use = dso_local unnamed_addr alias void (%cM*), void (%cM*)* @foo

define dso_local void @foo(%cM* %this) unnamed_addr align 2 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !prof !0 {
entry:
  %0 = load void (%cE, %cM*)*, void (%cE, %cM*)** undef, align 8
  invoke void %0(%cE undef, %cM* %this)
          to label %cond.end unwind label %lpad

cond.end:                                         ; preds = %entry
  invoke void @bar()
          to label %for.cond unwind label %lpad22.loopexit.split-lp, !prof !1

for.cond:                             ; preds = %cond.end
  unreachable

lpad:                                             ; preds = %entry
  %1 = landingpad { i8*, i32 }
          catch i8* null
  unreachable

lpad22.loopexit.split-lp:                         ; preds = %cond.end
  %lpad.loopexit.split-lp = landingpad { i8*, i32 }
          catch i8* null
  unreachable
}

declare i32 @__gxx_personality_v0(...)

declare dso_local void @bar() local_unnamed_addr align 2

!0 = !{!"function_entry_count", i64 9}
!1 = !{!"branch_weights", i32 9, i32 0}

The verifier throws:

Wrong number of operands
!1 = !{!"branch_weights", i32 9, i32 0}

The issue is that SimplifyCFG tries to replace an invoke with a call, and copies over the MD as is.
One quick fix is

--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -749,9 +749,14 @@ void Instruction::copyMetadata(const Instruction &SrcInst,
   // new one.
   SmallVector<std::pair<unsigned, MDNode *>, 4> TheMDs;
   SrcInst.getAllMetadataOtherThanDebugLoc(TheMDs);
+  bool fixupBranchWeight = isa<CallInst>(this) && isa<InvokeInst>(SrcInst);
   for (const auto &MD : TheMDs) {
-    if (WL.empty() || WLS.count(MD.first))
-      setMetadata(MD.first, MD.second);
+    if (!WL.empty() && !WLS.count(MD.first))
+      continue;
+    MDNode *MDN = MD.second;
+    if (fixupBranchWeight && MDN->getNumOperands() == 3)
+      MDN = MDNode::get(MDN->getContext(), {MDN->getOperand(0), MDN->getOperand(1)});
+    setMetadata(MD.first, MDN);
   }
   if (WL.empty() || WLS.count(LLVMContext::MD_dbg))
     setDebugLoc(SrcInst.getDebugLoc());

But i'm not sure if it's the best one so I'll leave to you to decide.

Thank you for your attention.

Thank you for reporting the case.
The bug is fixed with https://reviews.llvm.org/D81996 by Hans Wennborg.