Skip to content

Add unused type parameter to retain and release to prepare for closures #1287

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
May 27, 2020
Merged

Add unused type parameter to retain and release to prepare for closures #1287

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
42c1526
Add unused type parameter to retain and release to prepare for closures
DuncanUszkay1 May 22, 2020
6a08769
Change argument order for makeReplace
DuncanUszkay1 May 22, 2020
549ed73
fix type
DuncanUszkay1 May 22, 2020
1058545
Apply suggestions from code review
DuncanUszkay1 May 25, 2020
a862df5
Require a type to always be passed to retain/release
DuncanUszkay1 May 25, 2020
1fd8748
Apply PR comment changes
DuncanUszkay1 May 26, 2020
310b51c
remove old todo comment
DuncanUszkay1 May 26, 2020
2ed0693
fix ordering of types
DuncanUszkay1 May 26, 2020
633a9a3
Apply suggestions from code review
DuncanUszkay1 May 26, 2020
d61b7ea
fix make replace ordering
DuncanUszkay1 May 26, 2020
4d7dc30
lint fixes
DuncanUszkay1 May 26, 2020
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
Next Next commit
Add unused type parameter to retain and release to prepare for closures
  • Loading branch information
@DuncanUszkay1
DuncanUszkay1 committed May 26, 2020
commit 42c152611c65f288f8f64f09216d2d40eeded14b
95 changes: 54 additions & 41 deletions src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,7 @@ export class Compiler extends DiagnosticEmitter {
);
}
module.addGlobal(internalName, nativeType, true, this.makeZero(type));
if (type.isManaged && !this.skippedAutoreleases.has(initExpr)) initExpr = this.makeRetain(initExpr);
if (type.isManaged && !this.skippedAutoreleases.has(initExpr)) initExpr = this.makeRetain(initExpr, type);
this.currentBody.push(
module.global_set(internalName, initExpr)
);
Expand Down Expand Up @@ -1373,7 +1373,8 @@ export class Compiler extends DiagnosticEmitter {
stmts.push(
module.local_set(index,
this.makeRetain(
module.local_get(index, type.toNativeType())
module.local_get(index, type.toNativeType()),
type
)
)
);
Expand Down Expand Up @@ -1674,7 +1675,7 @@ export class Compiler extends DiagnosticEmitter {
module.local_get(0, nativeThisType),
nativeValueType, instance.memoryOffset
);
if (type.isManaged) valueExpr = this.makeRetain(valueExpr);
if (type.isManaged) valueExpr = this.makeRetain(valueExpr, type);
instance.getterRef = module.addFunction(instance.internalGetterName, nativeThisType, nativeValueType, null, valueExpr);
if (instance.setterRef) {
instance.set(CommonFlags.COMPILED);
Expand Down Expand Up @@ -1711,9 +1712,9 @@ export class Compiler extends DiagnosticEmitter {
),
module.block(null, [
module.drop(
this.makeRetain(module.local_get(1, nativeValueType))
this.makeRetain(module.local_get(1, nativeValueType), type)
),
this.makeRelease(module.local_get(2, nativeValueType))
this.makeRelease(module.local_get(2, nativeValueType), type)
])
),
module.local_get(1, nativeValueType)
Expand Down Expand Up @@ -3099,7 +3100,7 @@ export class Compiler extends DiagnosticEmitter {
module.local_set(local.index,
initAutoreleaseSkipped
? initExpr
: this.makeRetain(initExpr)
: this.makeRetain(initExpr, type)
)
);
} else {
Expand Down Expand Up @@ -3497,7 +3498,7 @@ export class Compiler extends DiagnosticEmitter {
// check if that worked, and if it didn't, keep the reference alive
if (!this.skippedAutoreleases.has(expr)) {
let index = this.tryUndoAutorelease(expr, flow);
if (index == -1) expr = this.makeRetain(expr);
if (index == -1) expr = this.makeRetain(expr, returnType);
this.skippedAutoreleases.add(expr);
}
}
Expand Down Expand Up @@ -5638,7 +5639,7 @@ export class Compiler extends DiagnosticEmitter {
if (!leftAutoreleaseSkipped) {
retainLeftInElse = true;
} else {
rightExpr = this.makeRetain(rightExpr);
rightExpr = this.makeRetain(rightExpr, rightType);
rightAutoreleaseSkipped = true;
}
} else if (!(constraints & Constraints.WILL_RETAIN)) { // otherwise keep right alive a little longer
Expand All @@ -5649,7 +5650,8 @@ export class Compiler extends DiagnosticEmitter {
if (leftAutoreleaseSkipped) { // left turned out to be true'ish and is dropped
rightStmts.unshift(
this.makeRelease(
module.local_get(temp.index, leftType.toNativeType())
module.local_get(temp.index, leftType.toNativeType()),
leftType
)
);
}
Expand All @@ -5662,7 +5664,8 @@ export class Compiler extends DiagnosticEmitter {
rightExpr,
retainLeftInElse
? this.makeRetain(
module.local_get(temp.index, leftType.toNativeType())
module.local_get(temp.index, leftType.toNativeType()),
leftType
)
: module.local_get(temp.index, leftType.toNativeType())
);
Expand Down Expand Up @@ -5742,7 +5745,7 @@ export class Compiler extends DiagnosticEmitter {
if (!leftAutoreleaseSkipped) {
retainLeftInThen = true;
} else {
rightExpr = this.makeRetain(rightExpr);
rightExpr = this.makeRetain(rightExpr, rightType);
rightAutoreleaseSkipped = true;
}
} else if (!(constraints & Constraints.WILL_RETAIN)) { // otherwise keep right alive a little longer
Expand All @@ -5755,7 +5758,8 @@ export class Compiler extends DiagnosticEmitter {
// once implicit conversion with strings is performed and left is "", so:
rightStmts.unshift(
this.makeRelease(
module.local_get(temp.index, leftType.toNativeType())
module.local_get(temp.index, leftType.toNativeType()),
rightType
)
);
}
Expand All @@ -5767,7 +5771,8 @@ export class Compiler extends DiagnosticEmitter {
this.makeIsTrueish(leftExpr, leftType),
retainLeftInThen
? this.makeRetain(
module.local_get(temp.index, leftType.toNativeType())
module.local_get(temp.index, leftType.toNativeType()),
leftType
)
: module.local_get(temp.index, leftType.toNativeType()),
rightExpr
Expand Down Expand Up @@ -6190,7 +6195,8 @@ export class Compiler extends DiagnosticEmitter {
valueExpr = this.makeReplace(
valueExpr,
module.local_get(localIndex, type.toNativeType()),
alreadyRetained
alreadyRetained,
type
);
if (tee) { // local = REPLACE(local, value)
this.currentType = type;
Expand All @@ -6202,7 +6208,7 @@ export class Compiler extends DiagnosticEmitter {
} else {
flow.unsetLocalFlag(localIndex, LocalFlags.CONDITIONALLY_RETAINED);
flow.setLocalFlag(localIndex, LocalFlags.RETAINED);
if (!alreadyRetained) valueExpr = this.makeRetain(valueExpr);
if (!alreadyRetained) valueExpr = this.makeRetain(valueExpr, type);
if (tee) { // local = __retain(value, local)
this.currentType = type;
return module.local_tee(localIndex, valueExpr);
Expand Down Expand Up @@ -6246,7 +6252,8 @@ export class Compiler extends DiagnosticEmitter {
this.makeReplace(
valueExpr,
module.global_get(global.internalName, nativeType),
alreadyRetained
alreadyRetained,
type
)
);
if (tee) { // (global = REPLACE(global, value))), global
Expand Down Expand Up @@ -6319,7 +6326,8 @@ export class Compiler extends DiagnosticEmitter {
module.local_get(tempThis.index, nativeThisType),
nativeFieldType, field.memoryOffset
),
alreadyRetained
alreadyRetained,
fieldType
),
nativeFieldType, field.memoryOffset
),
Expand All @@ -6336,7 +6344,8 @@ export class Compiler extends DiagnosticEmitter {
module.local_get(tempThis.index, nativeThisType),
nativeFieldType, field.memoryOffset
),
alreadyRetained
alreadyRetained,
fieldType
),
nativeFieldType, field.memoryOffset
);
Expand Down Expand Up @@ -6833,7 +6842,7 @@ export class Compiler extends DiagnosticEmitter {
if (flow.isNonnull(paramExpr, paramType)) flow.setLocalFlag(argumentLocal.index, LocalFlags.NONNULL);
// inlining is aware of skipped autoreleases:
if (paramType.isManaged) {
if (!this.skippedAutoreleases.has(paramExpr)) paramExpr = this.makeRetain(paramExpr);
if (!this.skippedAutoreleases.has(paramExpr)) paramExpr = this.makeRetain(paramExpr, paramType);
flow.setLocalFlag(argumentLocal.index, LocalFlags.RETAINED);
}
body.unshift(
Expand Down Expand Up @@ -6877,7 +6886,7 @@ export class Compiler extends DiagnosticEmitter {
if (flow.isNonnull(initExpr, initType)) flow.setLocalFlag(argumentLocal.index, LocalFlags.NONNULL);
if (initType.isManaged) {
flow.setLocalFlag(argumentLocal.index, LocalFlags.RETAINED);
if (!this.skippedAutoreleases.has(initExpr)) initExpr = this.makeRetain(initExpr);
if (!this.skippedAutoreleases.has(initExpr)) initExpr = this.makeRetain(initExpr, initType);
}
body.push(
module.local_set(argumentLocal.index, initExpr)
Expand All @@ -6901,7 +6910,7 @@ export class Compiler extends DiagnosticEmitter {
this.currentType = returnType;
if (returnType.isManaged) {
if (immediatelyDropped) {
expr = this.makeRelease(expr);
expr = this.makeRelease(expr, returnType);
this.currentType = Type.void;
}
}
Expand Down Expand Up @@ -7238,14 +7247,14 @@ export class Compiler extends DiagnosticEmitter {
// <reference-counting>

/** Makes a retain call, retaining the expression's value. */
makeRetain(expr: ExpressionRef): ExpressionRef {
makeRetain(expr: ExpressionRef, type: Type | null = null): ExpressionRef {
var retainInstance = this.program.retainInstance;
this.compileFunction(retainInstance);
return this.module.call(retainInstance.internalName, [ expr ], this.options.nativeSizeType);
}

/** Makes a release call, releasing the expression's value. Changes the current type to void.*/
makeRelease(expr: ExpressionRef): ExpressionRef {
makeRelease(expr: ExpressionRef, type: Type | null = null): ExpressionRef {
var releaseInstance = this.program.releaseInstance;
this.compileFunction(releaseInstance);
return this.module.call(releaseInstance.internalName, [ expr ], NativeType.None);
Expand All @@ -7259,6 +7268,8 @@ export class Compiler extends DiagnosticEmitter {
oldExpr: ExpressionRef,
/** Whether the new value is already retained. */
alreadyRetained: bool = false,
/** The type shared between expressions. */
type: Type
): ExpressionRef {
var module = this.module;
var flow = this.currentFlow;
Expand All @@ -7269,7 +7280,7 @@ export class Compiler extends DiagnosticEmitter {
let temp = flow.getTempLocal(this.options.usizeType, findUsedLocals(oldExpr));
let ret = module.block(null, [
module.local_set(temp.index, newExpr),
this.makeRelease(oldExpr),
this.makeRelease(oldExpr, type),
module.local_get(temp.index, nativeSizeType)
], nativeSizeType);
flow.freeTempLocal(temp);
Expand All @@ -7290,9 +7301,9 @@ export class Compiler extends DiagnosticEmitter {
),
module.block(null, [
module.local_set(temp1.index,
this.makeRetain(module.local_get(temp1.index, nativeSizeType))
this.makeRetain(module.local_get(temp1.index, nativeSizeType), type)
),
this.makeRelease(module.local_get(temp2.index, nativeSizeType))
this.makeRelease(module.local_get(temp2.index, nativeSizeType), type)
])
),
module.local_get(temp1.index, nativeSizeType)
Expand Down Expand Up @@ -7399,7 +7410,7 @@ export class Compiler extends DiagnosticEmitter {
// If it worked, autorelease in `outerFlow` instead
? this.makeAutorelease(expr, type, outerFlow)
// If it didn't work, extend the lifetime into `outerFlow`
: this.makeAutorelease(this.makeRetain(expr), type, outerFlow);
: this.makeAutorelease(this.makeRetain(expr, type), type, outerFlow);
}

/** Performs any queued autoreleases in the specified flow. */
Expand Down Expand Up @@ -7429,7 +7440,8 @@ export class Compiler extends DiagnosticEmitter {
if (finalize) flow.unsetLocalFlag(localIndex, LocalFlags.ANY_RETAINED);
stmts.push(
this.makeRelease(
module.local_get(localIndex, local.type.toNativeType())
module.local_get(localIndex, local.type.toNativeType()),
local.type
)
);
}
Expand Down Expand Up @@ -7524,7 +7536,8 @@ export class Compiler extends DiagnosticEmitter {
flow.unsetLocalFlag(localIndex, LocalFlags.ANY_RETAINED);
stmts.push(
this.makeRelease(
module.local_get(localIndex, local.type.toNativeType())
module.local_get(localIndex, local.type.toNativeType()),
local.type
)
);
}
Expand Down Expand Up @@ -7661,7 +7674,7 @@ export class Compiler extends DiagnosticEmitter {
this.currentType = returnType;
if (returnType.isManaged) {
if (immediatelyDropped) {
expr = this.makeRelease(expr);
expr = this.makeRelease(expr, returnType);
this.currentType = Type.void;
} else if (!skipAutorelease) {
expr = this.makeAutorelease(expr, returnType);
Expand All @@ -7687,7 +7700,7 @@ export class Compiler extends DiagnosticEmitter {
this.currentType = returnType;
if (returnType.isManaged) {
if (immediatelyDropped) {
expr = this.makeRelease(expr);
expr = this.makeRelease(expr, returnType);
this.currentType = Type.void;
} else if (!skipAutorelease) {
expr = this.makeAutorelease(expr, returnType);
Expand Down Expand Up @@ -7800,7 +7813,7 @@ export class Compiler extends DiagnosticEmitter {
this.currentType = returnType;
if (returnType.isManaged) {
if (immediatelyDropped) {
expr = this.makeRelease(expr);
expr = this.makeRelease(expr, returnType);
this.currentType = Type.void;
} else {
expr = this.makeAutorelease(expr, returnType);
Expand Down Expand Up @@ -8576,7 +8589,7 @@ export class Compiler extends DiagnosticEmitter {
: module.i32(i64_low(bufferAddress))
], expression);
this.currentType = arrayType;
expr = this.makeRetain(expr);
expr = this.makeRetain(expr, elementType);
if (arrayType.isManaged) {
if (!(constraints & Constraints.WILL_RETAIN)) {
expr = this.makeAutorelease(expr, arrayType);
Expand Down Expand Up @@ -8795,7 +8808,7 @@ export class Compiler extends DiagnosticEmitter {
if (isManaged) {
// value = __retain(value)
if (!this.skippedAutoreleases.has(valueExpr)) {
valueExpr = this.makeRetain(valueExpr);
valueExpr = this.makeRetain(valueExpr, elementType);
}
}
// store<T>(tempThis, value, immOffset)
Expand Down Expand Up @@ -8921,7 +8934,7 @@ export class Compiler extends DiagnosticEmitter {

let expr = this.compileExpression(values[i], fieldType, Constraints.CONV_IMPLICIT | Constraints.WILL_RETAIN);
if (fieldType.isManaged && !this.skippedAutoreleases.has(expr)) {
expr = this.makeRetain(expr);
expr = this.makeRetain(expr, fieldType);
}
exprs.push(
module.store( // TODO: handle setters as well
Expand Down Expand Up @@ -9390,18 +9403,18 @@ export class Compiler extends DiagnosticEmitter {

if (ifThenAutoreleaseSkipped != ifElseAutoreleaseSkipped) { // unify to both skipped
if (!ifThenAutoreleaseSkipped) {
ifThenExpr = this.makeRetain(ifThenExpr);
ifThenExpr = this.makeRetain(ifThenExpr, ifThenType);
ifThenAutoreleaseSkipped = true;
} else {
ifElseExpr = this.makeRetain(ifElseExpr);
ifElseExpr = this.makeRetain(ifElseExpr, ifElseType);
ifElseAutoreleaseSkipped = true;
}
} else if (!ifThenAutoreleaseSkipped && commonType.isManaged) { // keep alive a little longer
if (constraints & Constraints.WILL_RETAIN) { // try to undo both
let ifThenIndex = this.tryUndoAutorelease(ifThenExpr, ifThenFlow);
if (ifThenIndex == -1) ifThenExpr = this.makeRetain(ifThenExpr);
if (ifThenIndex == -1) ifThenExpr = this.makeRetain(ifThenExpr, ifThenType);
let ifElseIndex = this.tryUndoAutorelease(ifElseExpr, ifElseFlow);
if (ifElseIndex == -1) ifElseExpr = this.makeRetain(ifElseExpr);
if (ifElseIndex == -1) ifElseExpr = this.makeRetain(ifElseExpr, ifElseType);
ifThenAutoreleaseSkipped = true;
ifElseAutoreleaseSkipped = true;
} else {
Expand Down Expand Up @@ -10472,15 +10485,15 @@ export class Compiler extends DiagnosticEmitter {
: 1 + parameterIndex, // this is local 0
nativeFieldType
);
if (fieldType.isManaged) initExpr = this.makeRetain(initExpr);
if (fieldType.isManaged) initExpr = this.makeRetain(initExpr, fieldType);

// fall back to use initializer if present
} else if (initializerNode) {
initExpr = this.compileExpression(initializerNode, fieldType,
Constraints.CONV_IMPLICIT | Constraints.WILL_RETAIN
);
if (fieldType.isManaged && !this.skippedAutoreleases.has(initExpr)) {
initExpr = this.makeRetain(initExpr);
initExpr = this.makeRetain(initExpr, fieldType);
}

// otherwise initialize with zero
Expand Down