Eric Christopher <[email protected]> writes:
> Just a few things here...
>
> + CGF.EmitBranchOnBoolExpr(E->getConfig(), ContBlock, ConfigOKBlock, 0);
>
> Entirely up to you, but I find that commenting options like the 0 there with
> "Initial value" or something is useful when you're not passing an obvious
> argument along that has a name.
Sure.
> + if (CondExprBool)
> + Cnt.beginRegion(Builder);
>
> I'd probably want a comment above the conditional here.
Sure.
> + PGO.setCurrentRegionCount(0);
>
> Perhaps resetCurrentRegionCount to do the explicit 0? It's more obvious what's
> going on then.
I had that in a previous iteration, and explicitly setting the count
ends up clearer. Reset is ambiguous, whereas "the region count is zero
here" clearly indicates this region shouldn't occur.
> + Cnt.adjustFallThroughCount();
>
> This is weird - I'll mention it later :)
>
> + uint64_t Total = CaseCnt.getCount() - CaseCnt.getParentCount();
> + unsigned NCases = Range.getZExtValue() + 1;
> + uint64_t Weight = Total / NCases, Rem = Total % NCases;
> + for (unsigned I = 0; I != NCases; ++I) {
> + if (SwitchWeights)
> + SwitchWeights->push_back(Weight + (Rem ? 1 : 0));
> + if (Rem)
> + Rem--;
> SwitchInsn->addCase(Builder.getInt(LHS), CaseDest);
>
> Should comment what you're doing here.
Sure.
> + (*SwitchWeights)[0] += ThisCount;
>
> This seems awkward.
That's why there's a big comment explaining it.
> + llvm::BasicBlock *SkipCountBB = 0;
> + if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
> + SkipCountBB = createBasicBlock("skipcount");
> + EmitBranch(SkipCountBB);
> + }
>
> Probably want a comment of why you're emitting this block.
Sure.
> + const SwitchCase *Case = S.getSwitchCaseList();
> + uint64_t DefaultCount = 0;
> + unsigned NumCases = 0;
> + for (; Case; Case = Case->getNextSwitchCase()) {
>
> No reason to have Case outside of the loop as far as I can tell?
Sure.
> - llvm::BasicBlock *FalseBlock);
> + llvm::BasicBlock *FalseBlock, uint64_t
> TrueCount);
>
> Comment what TrueCount is above.
Sure.
> + void emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter) {
> + if (!CGM.getCodeGenOpts().ProfileInstrGenerate)
> + return;
> + llvm::Value *Addr =
> + Builder.CreateConstInBoundsGEP2_64(RegionCounters, 0, Counter);
> + llvm::Value *Count = Builder.CreateLoad(Addr, "pgocount");
> + Count = Builder.CreateAdd(Count, Builder.getInt64(1));
> + Builder.CreateStore(Count, Addr);
> + }
>
> Probably a bit big to have in the header.
Sure, moved.
> + /// Get the value of the counter with adjustments applied. Adjustments are
> + /// applied whenever control flow enters or leaves the region abnormally.
> + uint64_t getAdjustedCount() const {
> + assert(Adjust > 0 || (uint64_t)(-Adjust) <= Count && "Negative count");
> + return Count + Adjust;
> + }
>
> Few things with this:
>
> a) Adjustments seem awkward, possibly need to describe this better.
> b) You use accessor functions in all of the other functions.
I've improved the comment. Adjust doesn't have an accessor, since it
isn't public, and mixing getCount() with Adjust is awkward.
> + /// Get the value of the counter that was active before this one.
> + uint64_t getParentCount() const { return ParentCount; }
>
> Parent count seems a bit awkward for naming. I've seen the uses through the
> source, but some documentation of what's going on here and why you'd want to
> get it would be good.
I've improved the comment.
> + /// Get the associated break counter. Undefined if this counter is not
> + /// counting a loop.
> + RegionCounter getBreakCounter() const {
> + return RegionCounter(*PGO, Counter + 1);
> + }
> + /// Get the associated continue counter. Undefined if this counter is not
> + /// counting a loop.
> + RegionCounter getContinueCounter() const {
> + return RegionCounter(*PGO, Counter + 2);
> + }
>
> "Undefined" boo! Any way to make it do something sane or assert?
It's possible to make this assert, but it's a bit messy. It's pretty
likely that these will go away completely in follow on work, so I think
it's best to address this outside of this review.
> + void setFallThroughCount() {
>
> Possibly "finishAdjustments" or something like that? A set that doesn't take
> an argument is a bit odd. The "adjustFallThroughCount()" code is also weird. I
> like abstracting the idea out, but I'm not sure how it should be done here.
I went with applyAdjustmentsToRegion.
> The testcases look much better. I'm a bit astounded that you can map them to
> the source quite like that, but hey, awesome that you can.
>
> Perhaps having a comment operator in the input file format would be nice so
> that you could comment the input files, but that's definitely an "in the
> future" sort of thing.
>
> -eric
I've attached a patch with just the incremental changes here, and as you
can see, there aren't any functional changes. Since we're basically just
improving comments at this point, and since this not being in has been
blocking other people trying to use and build on top of these changes,
I'm going to go ahead and commit now. I'll continue to incorporate
further feedback in follow on commits.
Thanks for the review!
diff --git a/lib/CodeGen/CGCUDARuntime.cpp b/lib/CodeGen/CGCUDARuntime.cpp
index 524db7a..54a28f5 100644
--- a/lib/CodeGen/CGCUDARuntime.cpp
+++ b/lib/CodeGen/CGCUDARuntime.cpp
@@ -31,7 +31,8 @@ RValue CGCUDARuntime::EmitCUDAKernelCallExpr(CodeGenFunction &CGF,
llvm::BasicBlock *ContBlock = CGF.createBasicBlock("kcall.end");
CodeGenFunction::ConditionalEvaluation eval(CGF);
- CGF.EmitBranchOnBoolExpr(E->getConfig(), ContBlock, ConfigOKBlock, 0);
+ CGF.EmitBranchOnBoolExpr(E->getConfig(), ContBlock, ConfigOKBlock,
+ /*TrueCount=*/0);
eval.begin(CGF);
CGF.EmitBlock(ConfigOKBlock);
diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp
index 0e7d418..9c108e9 100644
--- a/lib/CodeGen/CGExpr.cpp
+++ b/lib/CodeGen/CGExpr.cpp
@@ -2660,6 +2660,7 @@ EmitConditionalOperatorLValue(const AbstractConditionalOperator *expr) {
if (!CondExprBool) std::swap(live, dead);
if (!ContainsLabel(dead)) {
+ // If the true case is live, we need to track its region
if (CondExprBool)
Cnt.beginRegion(Builder);
return EmitLValue(live);
@@ -2699,7 +2700,7 @@ EmitConditionalOperatorLValue(const AbstractConditionalOperator *expr) {
rhsBlock = Builder.GetInsertBlock();
EmitBlock(contBlock);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
llvm::PHINode *phi = Builder.CreatePHI(lhs.getAddress()->getType(), 2,
"cond-lvalue");
diff --git a/lib/CodeGen/CGExprAgg.cpp b/lib/CodeGen/CGExprAgg.cpp
index fe2b9c7..1a6274e 100644
--- a/lib/CodeGen/CGExprAgg.cpp
+++ b/lib/CodeGen/CGExprAgg.cpp
@@ -923,7 +923,7 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
eval.end(CGF);
CGF.EmitBlock(ContBlock);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
}
void AggExprEmitter::VisitChooseExpr(const ChooseExpr *CE) {
diff --git a/lib/CodeGen/CGExprComplex.cpp b/lib/CodeGen/CGExprComplex.cpp
index 9bab761..145d946 100644
--- a/lib/CodeGen/CGExprComplex.cpp
+++ b/lib/CodeGen/CGExprComplex.cpp
@@ -773,7 +773,7 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
RHSBlock = Builder.GetInsertBlock();
CGF.EmitBlock(ContBlock);
eval.end(CGF);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
// Create a PHI node for the real part.
llvm::PHINode *RealPN = Builder.CreatePHI(LHS.first->getType(), 2, "cond.r");
diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp
index 9cc5946..a38c694 100644
--- a/lib/CodeGen/CGExprScalar.cpp
+++ b/lib/CodeGen/CGExprScalar.cpp
@@ -2947,7 +2947,7 @@ Value *ScalarExprEmitter::VisitBinLAnd(const BinaryOperator *E) {
Builder.SetCurrentDebugLocation(llvm::DebugLoc());
CGF.EmitBlock(ContBlock);
PN->addIncoming(RHSCond, RHSBlock);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
// ZExt result to int.
return Builder.CreateZExtOrBitCast(PN, ResTy, "land.ext");
@@ -3028,7 +3028,7 @@ Value *ScalarExprEmitter::VisitBinLOr(const BinaryOperator *E) {
// into the phi node for the edge with the value of RHSCond.
CGF.EmitBlock(ContBlock);
PN->addIncoming(RHSCond, RHSBlock);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
// ZExt result to int.
return Builder.CreateZExtOrBitCast(PN, ResTy, "lor.ext");
@@ -3185,7 +3185,7 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
RHSBlock = Builder.GetInsertBlock();
CGF.EmitBlock(ContBlock);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
// If the LHS or RHS is a throw expression, it will be legitimately null.
if (!LHS)
diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index db3d2532..5031d60 100644
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -503,7 +503,7 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
// Emit the continuation block for code after the if.
EmitBlock(ContBlock, true);
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
}
void CodeGenFunction::EmitWhileStmt(const WhileStmt &S) {
@@ -999,6 +999,9 @@ void CodeGenFunction::EmitCaseStmtRange(const CaseStmt &S) {
// Range is small enough to add multiple switch instruction cases.
uint64_t Total = CaseCnt.getCount() - CaseCnt.getParentCount();
unsigned NCases = Range.getZExtValue() + 1;
+ // Divide the weights evenly between the cases, ensuring that the total
+ // weight is preserved. Ie, a weight of 5 over three cases will be
+ // distributed as weights of 2, 2, and 1.
uint64_t Weight = Total / NCases, Rem = Total % NCases;
for (unsigned I = 0; I != NCases; ++I) {
if (SwitchWeights)
@@ -1140,6 +1143,9 @@ void CodeGenFunction::EmitDefaultStmt(const DefaultStmt &S) {
llvm::BasicBlock *SkipCountBB = 0;
if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
+ // The PGO region here needs to count the number of times the edge occurs,
+ // so fallthrough into this case will jump past the region counter to the
+ // skipcount basic block.
SkipCountBB = createBasicBlock("skipcount");
EmitBranch(SkipCountBB);
}
@@ -1414,10 +1420,11 @@ void CodeGenFunction::EmitSwitchStmt(const SwitchStmt &S) {
SwitchInsn = Builder.CreateSwitch(CondV, DefaultBlock);
if (PGO.haveRegionCounts()) {
// Walk the SwitchCase list to find how many there are.
- const SwitchCase *Case = S.getSwitchCaseList();
uint64_t DefaultCount = 0;
unsigned NumCases = 0;
- for (; Case; Case = Case->getNextSwitchCase()) {
+ for (const SwitchCase *Case = S.getSwitchCaseList();
+ Case;
+ Case = Case->getNextSwitchCase()) {
if (isa<DefaultStmt>(Case))
DefaultCount = getPGORegionCounter(Case).getCount();
NumCases += 1;
diff --git a/lib/CodeGen/CodeGenFunction.cpp b/lib/CodeGen/CodeGenFunction.cpp
index f77e5be..fb668e4 100644
--- a/lib/CodeGen/CodeGenFunction.cpp
+++ b/lib/CodeGen/CodeGenFunction.cpp
@@ -925,7 +925,7 @@ void CodeGenFunction::EmitBranchOnBoolExpr(const Expr *Cond,
EmitBranchOnBoolExpr(CondBOp->getRHS(), TrueBlock, FalseBlock, TrueCount);
eval.end(*this);
Cnt.adjustFallThroughCount();
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
return;
}
@@ -971,7 +971,7 @@ void CodeGenFunction::EmitBranchOnBoolExpr(const Expr *Cond,
eval.end(*this);
Cnt.adjustFallThroughCount();
- Cnt.setFallThroughCount();
+ Cnt.applyAdjustmentsToRegion();
return;
}
diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h
index 2d23a3b..b06f81b 100644
--- a/lib/CodeGen/CodeGenFunction.h
+++ b/lib/CodeGen/CodeGenFunction.h
@@ -2431,6 +2431,8 @@ public:
/// EmitBranchOnBoolExpr - Emit a branch on a boolean condition (e.g. for an
/// if statement) to the specified blocks. Based on the condition, this might
/// try to simplify the codegen of the conditional based on the branch.
+ /// TrueCount should be the number of times we expect the condition to
+ /// evaluate to true based on PGO data.
void EmitBranchOnBoolExpr(const Expr *Cond, llvm::BasicBlock *TrueBlock,
llvm::BasicBlock *FalseBlock, uint64_t TrueCount);
diff --git a/lib/CodeGen/CodeGenPGO.cpp b/lib/CodeGen/CodeGenPGO.cpp
index 5e84952..d1b57d8 100644
--- a/lib/CodeGen/CodeGenPGO.cpp
+++ b/lib/CodeGen/CodeGenPGO.cpp
@@ -398,6 +398,16 @@ void CodeGenPGO::emitCounterVariables() {
"__llvm_pgo_ctr");
}
+void CodeGenPGO::emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter) {
+ if (!CGM.getCodeGenOpts().ProfileInstrGenerate)
+ return;
+ llvm::Value *Addr =
+ Builder.CreateConstInBoundsGEP2_64(RegionCounters, 0, Counter);
+ llvm::Value *Count = Builder.CreateLoad(Addr, "pgocount");
+ Count = Builder.CreateAdd(Count, Builder.getInt64(1));
+ Builder.CreateStore(Count, Addr);
+}
+
void CodeGenPGO::loadRegionCounts(GlobalDecl &GD, PGOProfileData *PGOData) {
// For now, ignore the counts from the PGO data file only if the number of
// counters does not match. This could be tightened down in the future to
diff --git a/lib/CodeGen/CodeGenPGO.h b/lib/CodeGen/CodeGenPGO.h
index be1e3b1..43dee86 100644
--- a/lib/CodeGen/CodeGenPGO.h
+++ b/lib/CodeGen/CodeGenPGO.h
@@ -98,15 +98,8 @@ private:
void loadRegionCounts(GlobalDecl &GD, PGOProfileData *PGOData);
void emitCounterVariables();
- void emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter) {
- if (!CGM.getCodeGenOpts().ProfileInstrGenerate)
- return;
- llvm::Value *Addr =
- Builder.CreateConstInBoundsGEP2_64(RegionCounters, 0, Counter);
- llvm::Value *Count = Builder.CreateLoad(Addr, "pgocount");
- Count = Builder.CreateAdd(Count, Builder.getInt64(1));
- Builder.CreateStore(Count, Addr);
- }
+ /// Emit code to increment the counter at the given index
+ void emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter);
/// Return the region counter for the given statement. This should only be
/// called on statements that have a dedicated counter.
@@ -116,6 +109,7 @@ private:
return (*RegionCounterMap)[S];
}
+ /// Return the region count for the counter at the given index.
uint64_t getRegionCount(unsigned Counter) {
if (!haveRegionCounts())
return 0;
@@ -149,13 +143,19 @@ public:
/// the region of the counter was entered, but for switch labels it's the
/// number of direct jumps to that label.
uint64_t getCount() const { return Count; }
- /// Get the value of the counter with adjustments applied. Adjustments are
- /// applied whenever control flow enters or leaves the region abnormally.
+ /// Get the value of the counter with adjustments applied. Adjustments occur
+ /// when control enters or leaves the region abnormally, ie, if there is a
+ /// jump to a label within the region, or if the function can return from
+ /// within the region. The adjusted count, then, is the value of the counter
+ /// at the end of the region.
uint64_t getAdjustedCount() const {
assert(Adjust > 0 || (uint64_t)(-Adjust) <= Count && "Negative count");
return Count + Adjust;
}
- /// Get the value of the counter that was active before this one.
+ /// Get the value of the counter in this region's parent, ie, the region that
+ /// was active when this region began. This is useful for deriving counts in
+ /// implicitly counted regions, like the false case of a condition or the
+ /// normal exits of a loop.
uint64_t getParentCount() const { return ParentCount; }
/// Get the number of times the condition of a loop will evaluate false. This
@@ -205,7 +205,7 @@ public:
}
/// Commit all adjustments to the current region. This should be called after
/// all blocks that adjust the fallthrough count have been emitted.
- void setFallThroughCount() {
+ void applyAdjustmentsToRegion() {
PGO->setCurrentRegionCount(ParentCount + Adjust);
}
};
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
