Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing UnrolledLoopStatement to PGO region count calculator. #3524

Merged
merged 1 commit into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 74 additions & 5 deletions gen/pgo_ASTbased.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class PGOHash {
ConditionalExpr,
AndAndExpr,
OrOrExpr,
UnrolledLoopIterationScope,

// Keep this last. It's for the static assert that follows.
LastHashType
Expand Down Expand Up @@ -176,11 +177,39 @@ struct MapRegionCounters : public StoppableVisitor {
} \
} while (0)

void visit(Statement *stmt) override {}
void visit(Expression *exp) override {}
void visit(Declaration *decl) override {}
void visit(Initializer *init) override {}
void visit(Dsymbol *dsym) override {}
void visit(Statement *stmt) override {
// If this assert fails, a new statement type was added to the frontend, and
// thus we need to decide on how to handle PGO calculations for that, both
// in MapRegionCounters and in ComputeRegionCounts.
assert(0 && "All statement types should be explicitly handled to avoid "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to do something in addition or other than assert so that detection is explicit in release mode builds that end users will use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. The error may not be user-actionable, and things may also just work without change needed. Would you suggest a warning saying that "Statement on line x is not supported by PGO" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note that this is the same for CaseRangeStatement)

Copy link
Contributor

@jondegenhardt jondegenhardt Jul 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you suggest a warning saying that "Statement on line x is not supported by PGO" ?

Something like that. Perhaps requesting that an issue be filed for the purpose of improving LDC. This might make more sense if similar things are already being done elsewhere in the code base.

This rationale is that it might be difficult to catch these cases in the context of LDC development, and instead they'd be more likely to occur when running against user code. Do you think this is the case? If not, then such a message is not needed.

The other part is that PGO computation should proceed reasonably even if the assert condition arises. Ie. Not crash. I didn't try to review the code from this angle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I disregard this? We have many asserts in the codebase where we don't really report it to the user. (I think it is not very valuable for the user, to be honest) I do agree that the assert is not very likely to catch things during development, but it might when people use the beta build or latest-ci to have asserts enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I disregard this?

I don't mind. It was a suggestion for your consideration, nothing else. And it makes complete sense to be consistent with similar cases in the LDC code base.

"missing new statement types in MapRegionCounters and "
"ComputeRegionCounts");
}
void visit(CompoundStatement *) override {}
void visit(ExpStatement *) override {}
void visit(ImportStatement *) override {}
void visit(ScopeStatement *) override {}
void visit(ReturnStatement *) override {}
void visit(StaticAssertStatement *) override {}
void visit(CompileStatement *) override {}
void visit(ScopeGuardStatement *) override {}
void visit(ConditionalStatement *) override {}
void visit(StaticForeachStatement *) override {}
void visit(PragmaStatement *) override {}
void visit(BreakStatement *) override {}
void visit(ContinueStatement *) override {}
void visit(GotoDefaultStatement *) override {}
void visit(GotoCaseStatement *) override {}
void visit(GotoStatement *) override {}
void visit(SynchronizedStatement *) override {}
void visit(WithStatement *) override {}
void visit(ThrowStatement *) override {}
void visit(AsmStatement *) override {}

void visit(Expression *) override {}
void visit(Declaration *) override {}
void visit(Initializer *) override {}
void visit(Dsymbol *) override {}

void visit(FuncDeclaration *fd) override {
if (NextCounter) {
Expand Down Expand Up @@ -229,6 +258,23 @@ struct MapRegionCounters : public StoppableVisitor {
Hash.combine(PGOHash::ForeachRangeStmt);
}

void visit(UnrolledLoopStatement *stmt) override {
// Continues and breaks in the scopes of UnrolledLoop iterations can "goto"
// to "labels" at the start of the next iteration or end of the loop. We
// should therefore treat each unrolled loop iteration the same as
// LabelStatements (with redundant counting of the first iteration which is
// always executed).
// The counter for the UnrolledLoopStatement itself counts the
// exit block of the 'loop'.
SKIP_VISITED(stmt);
CounterMap[stmt] = NextCounter++;
Hash.combine(PGOHash::UnrolledLoopIterationScope);
for (auto s : *stmt->statements) {
CounterMap[s] = NextCounter++;
Hash.combine(PGOHash::UnrolledLoopIterationScope);
}
}

void visit(LabelStatement *stmt) override {
SKIP_VISITED(stmt);
CounterMap[stmt] = NextCounter++;
Expand Down Expand Up @@ -452,6 +498,29 @@ struct ComputeRegionCounts : public RecursiveVisitor {
RecordNextStmtCount = true;
}

void visit(UnrolledLoopStatement *S) override {
RecordStmtCount(S);

// The iterations can still have `break` and `continue` even though we are
// no longer in a loop. We need to provide this BreakContinue struct for
// those breaks&continues to refer to, but we do not use it otherwise.
BreakContinueStack.push_back(BreakContinue());

// Iteration statement counters track the entry block of each iteration
// (redundant for first iteration)
for (auto iteration_stmt : *S->statements) {
setCount(PGO.getRegionCount(iteration_stmt));
RecordNextStmtCount = true;
recurse(iteration_stmt);
}

BreakContinueStack.pop_back();

// UnrolledLoopStatement counter tracks the continuation after the statement.
setCount(PGO.getRegionCount(S));
RecordNextStmtCount = true;
}

void visit(WhileStatement *S) override {
RecordStmtCount(S);
uint64_t ParentCount = CurrentCount;
Expand Down
5 changes: 5 additions & 0 deletions gen/statements.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,8 @@ class ToIRVisitor : public Visitor {
// continue goes to next statement, break goes to end
irs->funcGen().jumpTargets.pushLoopTarget(stmt, nextbb, endbb);

PGO.emitCounterIncrement(s);

// do statement
s->accept(this);

Expand All @@ -1251,6 +1253,9 @@ class ToIRVisitor : public Visitor {

irs->scope() = IRScope(endbb);

// PGO counter tracks the continuation after the loop
PGO.emitCounterIncrement(stmt);

// end the dwarf lexical block
irs->DBuilder.EmitBlockEnd();
}
Expand Down
84 changes: 84 additions & 0 deletions tests/PGO/unrolledloopstatement_gh3375.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Test PGO instrumentation and profile use for front-end-unrolled loops.

// REQUIRES: PGO_RT

// RUN: %ldc -fprofile-instr-generate=%t.profraw -run %s
// RUN: %profdata merge %t.profraw -o %t.profdata
// RUN: %ldc -c -output-ll -of=%t2.ll -fprofile-instr-use=%t.profdata %s
// RUN: FileCheck %allow-deprecated-dag-overlap %s -check-prefix=PROFUSE < %t2.ll

alias AliasSeq(TList...) = TList;

void main() {
foreach (i; 0..400)
foofoofoo(i);
}

// PROFUSE-LABEL: define void @foofoofoo(
// PROFUSE-SAME: !prof ![[FUNCENTRY:[0-9]+]]
extern(C) void foofoofoo(int i)
{
alias R = AliasSeq!(char, int);
foreach (j, r; R)
{
if (i + 125*j > 200)
continue;

if (i + 125*j > 150)
break;

if (i-j == 0)
goto function_exit;
}
/* The loop will be unrolled to:
{
// Here: i in [0..399] = 400 counts
// PROFUSE: br {{.*}} !prof ![[IF1_1:[0-9]+]]
if (i + 0 > 200)
continue; // [201..399] = 199 counts

// [0..200] = 201 counts
// PROFUSE: br {{.*}} !prof ![[IF1_2:[0-9]+]]
if (i + 0 > 150)
break; // [151..200] = 50 counts

// [0..150] = 151 counts
// PROFUSE: br {{.*}} !prof ![[IF1_3:[0-9]+]]
if (i-0 == 0)
goto function_exit;
}
{
// [1..150] U [201..399] = 150+199 = 349 counts
// PROFUSE: br {{.*}} !prof ![[IF2_1:[0-9]+]]
if (i + 125 > 200)
continue; // [76..150] U [201..399] = 75+199 = 274 counts

// [1..75] = 75 counts
// PROFUSE: br {{.*}} !prof ![[IF2_2:[0-9]+]]
if (i + 125 > 150)
break; // [26..75] = 50 counts

// [1..25] = 25 counts
// PROFUSE: br {{.*}} !prof ![[IF2_3:[0-9]+]]
if (i-1 == 0)
goto function_exit;
}
*/

// [2..400] = 398 counts
// PROFUSE: br {{.*}} !prof ![[IFEXIT:[0-9]+]]
if (i) {} // always true

// 400 counts
function_exit:
}

// PROFUSE-DAG: ![[FUNCENTRY]] = !{!"function_entry_count", i64 400}
// PROFUSE-DAG: ![[IF1_1]] = !{!"branch_weights", i32 200, i32 202}
// PROFUSE-DAG: ![[IF1_2]] = !{!"branch_weights", i32 51, i32 152}
// PROFUSE-DAG: ![[IF1_3]] = !{!"branch_weights", i32 2, i32 151}
// PROFUSE-DAG: ![[IF2_1]] = !{!"branch_weights", i32 275, i32 76}
// PROFUSE-DAG: ![[IF2_2]] = !{!"branch_weights", i32 51, i32 26}
// PROFUSE-DAG: ![[IF2_3]] = !{!"branch_weights", i32 2, i32 25}
// PROFUSE-DAG: ![[IFEXIT]] = !{!"branch_weights", i32 399, i32 1}