Skip to content

Commit

Permalink
[DAPHNE-#911] Correct reference counter for arith.select result.
Browse files Browse the repository at this point in the history
- The MLIR operation arith.select is frequently created during the canonicalization of scf.if (which, in turn, is created by DaphneDSL if-statements).
- arith.select takes a boolean argument (condition) plus two additional arguments, one of which it selects/returns based on the value of the condition.
- arith.select needs to be taken into account in DAPHNE's object reference counter management, because:
  - We decrease the reference counter of SSA values after their last use as operands (the arith.select could be the last user).
  - We have no clue which of the two last operands of arith.select is returned at run-time.
  - However, the return value is the same runtime object as one of the two latter operands.
  - To ensure the correct reference counter for the result after the arith.select, we need to increase the counter of the result before we decrease the counters of the operands.
- We had already implemented this fix, but applied it only for string operands/results.
- However, we must apply the fix for all operand/result types that have a reference counter at run-time.
- Fixed ManageObjRefPass.
- Added several script-level test cases testing the behavior for matrices, frames, lists, and strings and different settings of the if-then-else in DaphneDSL.
  • Loading branch information
pdamme committed Nov 12, 2024
1 parent 287f4c5 commit aca0185
Show file tree
Hide file tree
Showing 26 changed files with 177 additions and 6 deletions.
15 changes: 10 additions & 5 deletions src/compiler/lowering/ManageObjRefsPass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ void processValue(OpBuilder builder, Value v) {
// TODO Address handles from the distributed runtime (but they will be
// removed soon anyway).
// We only need to manage the reference counters of DAPHNE data objects
// like matrices and frames (not of scalars).
// (like matrices, frames, and lists), but not for scalars (except for
// strings) and other types.

Operation *defOp = v.getDefiningOp();
if (defOp && llvm::isa<daphne::ConvertDenseMatrixToMemRef>(defOp))
Expand All @@ -96,12 +97,16 @@ void processValue(OpBuilder builder, Value v) {
builder.create<daphne::IncRefOp>(v.getLoc(), v);
}

// Increase the reference counter of the result of the arith.select op, if
// it is a string scalar. This is necessary because for arith.select, we
// have no clue which of its two arguments (2nd or 3rd one) it will return.
// Increase the reference counter of the result of an arith.select op,
// if the result is of a type that has a reference counter at runtime.
// This is necessary because for arith.select, we have no clue which of
// its two arguments (2nd or 3rd one) it will return.
// Unless we do something about it, the reference counter of the result will
// be too low by 1. Thus, we increase the result's reference counter here.
if (defOp && llvm::isa<arith::SelectOp>(defOp) && llvm::isa<daphne::StringType>(v.getType())) {
// TODO This might cause a memory leak, if this arith.select is not the last
// user of both input SSA values (further investigation necessary).
if (defOp && llvm::isa<arith::SelectOp>(defOp) &&
llvm::isa<daphne::MatrixType, daphne::FrameType, daphne::ListType, daphne::StringType>(v.getType())) {
builder.setInsertionPointAfter(defOp);
builder.create<daphne::IncRefOp>(v.getLoc(), v);
}
Expand Down
2 changes: 1 addition & 1 deletion test/api/cli/controlflow/ControlFlowTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const std::string dirPath = "test/api/cli/controlflow/";
} \
}

MAKE_TEST_CASE("if", 8)
MAKE_TEST_CASE("if", 20)
MAKE_TEST_CASE("for", 23)
MAKE_TEST_CASE("while", 17)
MAKE_TEST_CASE("nested", 26)
Expand Down
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_10.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a matrix inside else-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = [0];
Y = X + 1;
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
r = 99; # do something unrelated to X
else
X = Y;
print(r);
print(X);
3 changes: 3 additions & 0 deletions test/api/cli/controlflow/if_10.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
99
DenseMatrix(1x1, int64_t)
0
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_11.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a matrix inside then-branch and else-branch of if-statement with values computed outside if-statement (scf.if gets rewritten to arith.select).
X = [0];
Y = X + 1;
Z = X + 2;
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
else
X = Z;
print(X);
2 changes: 2 additions & 0 deletions test/api/cli/controlflow/if_11.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
DenseMatrix(1x1, int64_t)
1
9 changes: 9 additions & 0 deletions test/api/cli/controlflow/if_12.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Update a frame inside then-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = {"a": [0]};
Y = rbind(X, X);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
print(X);
3 changes: 3 additions & 0 deletions test/api/cli/controlflow/if_12.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Frame(2x1, [a:int64_t])
0
0
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_13.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a frame inside else-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = {"a": [0]};
Y = rbind(X, X);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
r = 99; # do something unrelated to X
else
X = Y;
print(r);
print(X);
3 changes: 3 additions & 0 deletions test/api/cli/controlflow/if_13.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
99
Frame(1x1, [a:int64_t])
0
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_14.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a frame inside then-branch and else-branch of if-statement with values computed outside if-statement (scf.if gets rewritten to arith.select).
X = {"a": [0]};
Y = rbind(X, X);
Z = rbind(rbind(X, X), X);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
else
X = Z;
print(X);
3 changes: 3 additions & 0 deletions test/api/cli/controlflow/if_14.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Frame(2x1, [a:int64_t])
0
0
9 changes: 9 additions & 0 deletions test/api/cli/controlflow/if_15.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Update a list inside then-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = createList([0]);
Y = append(X, [1]);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
print(X);
5 changes: 5 additions & 0 deletions test/api/cli/controlflow/if_15.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
List(2, DenseMatrix, int64_t)
DenseMatrix(1x1, int64_t)
0
DenseMatrix(1x1, int64_t)
1
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_16.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a list inside else-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = createList([0]);
Y = append(X, [1]);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
r = 99; # do something unrelated to X
else
X = Y;
print(r);
print(X);
4 changes: 4 additions & 0 deletions test/api/cli/controlflow/if_16.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
99
List(1, DenseMatrix, int64_t)
DenseMatrix(1x1, int64_t)
0
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_17.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a list inside then-branch and else-branch of if-statement with values computed outside if-statement (scf.if gets rewritten to arith.select).
X = createList([0]);
Y = append(X, [1]);
Z = append(X, [2]);
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
else
X = Z;
print(X);
5 changes: 5 additions & 0 deletions test/api/cli/controlflow/if_17.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
List(2, DenseMatrix, int64_t)
DenseMatrix(1x1, int64_t)
0
DenseMatrix(1x1, int64_t)
1
9 changes: 9 additions & 0 deletions test/api/cli/controlflow/if_18.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Update a string inside then-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = "a";
Y = X + "b";
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
print(X);
1 change: 1 addition & 0 deletions test/api/cli/controlflow/if_18.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ab
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_19.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a string inside else-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = "a";
Y = X + "b";
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
r = 99; # do something unrelated to X
else
X = Y;
print(r);
print(X);
2 changes: 2 additions & 0 deletions test/api/cli/controlflow/if_19.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
99
a
12 changes: 12 additions & 0 deletions test/api/cli/controlflow/if_20.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Update a string inside then-branch and else-branch of if-statement with values computed outside if-statement (scf.if gets rewritten to arith.select).
X = "a";
Y = X + "b";
Z = X + "c";
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
else
X = Z;
print(X);
1 change: 1 addition & 0 deletions test/api/cli/controlflow/if_20.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ab
9 changes: 9 additions & 0 deletions test/api/cli/controlflow/if_9.daphne
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Update a matrix inside then-branch of if-statement with value computed outside if-statement (scf.if gets rewritten to arith.select).
X = [0];
Y = X + 1;
# r must be unknown at compile-time (otherwise, the entire "if" gets optimized away)
# TODO The rand() might get optimized away at some point in the future (but not at the moment), because the result is clear.
r = as.scalar(rand(1, 1, 0, 1, 1, -1));
if(r)
X = Y;
print(X);
2 changes: 2 additions & 0 deletions test/api/cli/controlflow/if_9.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
DenseMatrix(1x1, int64_t)
1

0 comments on commit aca0185

Please sign in to comment.