-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
[compiler] Stay in SSA form through entire pipeline #30573
Changes from 26 commits
5b3dd3d
cf4bfb8
844b586
25b47d4
5a30b29
91d0553
f044d1a
124f022
cf3fc94
3589684
b2c6019
35015d6
8f87912
83c7429
d7c1a5f
c573bdf
ad9206b
79041f3
a1cddee
2e4b183
53cbd59
c6da502
a97e446
c053e9d
9d59031
65df4f8
8559ed8
512d6f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import {Environment, EnvironmentConfig, ExternalFunction} from '../HIR'; | |
import { | ||
ArrayPattern, | ||
BlockId, | ||
DeclarationId, | ||
GeneratedSource, | ||
Identifier, | ||
IdentifierId, | ||
|
@@ -309,9 +310,9 @@ function codegenReactiveFunction( | |
): Result<CodegenFunction, CompilerError> { | ||
for (const param of fn.params) { | ||
if (param.kind === 'Identifier') { | ||
cx.temp.set(param.identifier.id, null); | ||
cx.temp.set(param.identifier.declarationId, null); | ||
} else { | ||
cx.temp.set(param.place.identifier.id, null); | ||
cx.temp.set(param.place.identifier.declarationId, null); | ||
} | ||
} | ||
|
||
|
@@ -392,7 +393,11 @@ class Context { | |
env: Environment; | ||
fnName: string; | ||
#nextCacheIndex: number = 0; | ||
#declarations: Set<IdentifierId> = new Set(); | ||
/** | ||
* Tracks which named variables have been declared to dedupe declarations, | ||
* so this uses DeclarationId instead of IdentifierId | ||
*/ | ||
#declarations: Set<DeclarationId> = new Set(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is using the declarationId necessary in this pass? I'm having trouble reasoning about why it could break if we just used the identifier id--it looks like we'd be propagating the value of the SSA identifier through the temp map. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, without this we were generating duplicate variable declarations. For codegen what we care about is declaring program variables, not declaring SSA versions, so DeclarationId is the correct type here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized after our walkthrough today that I put this comment on the wrong place, lol. I think my actual question is why the temp map needs to use declarationIds--that's the part that seems like it should have the SSA id of a temporary's use match the SSA id of its definition. |
||
temp: Temporaries; | ||
errors: CompilerError = new CompilerError(); | ||
objectMethods: Map<IdentifierId, ObjectMethod> = new Map(); | ||
|
@@ -418,11 +423,11 @@ class Context { | |
} | ||
|
||
declare(identifier: Identifier): void { | ||
this.#declarations.add(identifier.id); | ||
this.#declarations.add(identifier.declarationId); | ||
} | ||
|
||
hasDeclared(identifier: Identifier): boolean { | ||
return this.#declarations.has(identifier.id); | ||
return this.#declarations.has(identifier.declarationId); | ||
} | ||
|
||
synthesizeName(name: string): ValidIdentifierName { | ||
|
@@ -1147,7 +1152,7 @@ function codegenTerminal( | |
let catchParam = null; | ||
if (terminal.handlerBinding !== null) { | ||
catchParam = convertIdentifier(terminal.handlerBinding.identifier); | ||
cx.temp.set(terminal.handlerBinding.identifier.id, null); | ||
cx.temp.set(terminal.handlerBinding.identifier.declarationId, null); | ||
} | ||
return t.tryStatement( | ||
codegenBlock(cx, terminal.block), | ||
|
@@ -1205,7 +1210,7 @@ function codegenInstructionNullable( | |
kind !== InstructionKind.Reassign && | ||
place.identifier.name === null | ||
) { | ||
cx.temp.set(place.identifier.id, null); | ||
cx.temp.set(place.identifier.declarationId, null); | ||
} | ||
const isDeclared = cx.hasDeclared(place.identifier); | ||
hasReasign ||= isDeclared; | ||
|
@@ -1261,7 +1266,7 @@ function codegenInstructionNullable( | |
); | ||
if (instr.lvalue !== null) { | ||
if (instr.value.kind !== 'StoreContext') { | ||
cx.temp.set(instr.lvalue.identifier.id, expr); | ||
cx.temp.set(instr.lvalue.identifier.declarationId, expr); | ||
return null; | ||
} else { | ||
// Handle chained reassignments for context variables | ||
|
@@ -1530,7 +1535,7 @@ function createCallExpression( | |
} | ||
} | ||
|
||
type Temporaries = Map<IdentifierId, t.Expression | t.JSXText | null>; | ||
type Temporaries = Map<DeclarationId, t.Expression | t.JSXText | null>; | ||
|
||
function codegenLabel(id: BlockId): string { | ||
return `bb${id}`; | ||
|
@@ -1549,7 +1554,7 @@ function codegenInstruction( | |
} | ||
if (instr.lvalue.identifier.name === null) { | ||
// temporary | ||
cx.temp.set(instr.lvalue.identifier.id, value); | ||
cx.temp.set(instr.lvalue.identifier.declarationId, value); | ||
return t.emptyStatement(); | ||
} else { | ||
const expressionValue = convertValueToExpression(value); | ||
|
@@ -2498,7 +2503,7 @@ function codegenPlaceToExpression(cx: Context, place: Place): t.Expression { | |
} | ||
|
||
function codegenPlace(cx: Context, place: Place): t.Expression | t.JSXText { | ||
let tmp = cx.temp.get(place.identifier.id); | ||
let tmp = cx.temp.get(place.identifier.declarationId); | ||
if (tmp != null) { | ||
return tmp; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we don't propagate phi types here -- do we not need them when analyzing functions? I guess our function mutability analysis is already a bit lacking 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. The phi types really only affect later passes like MergeScopesThatInvalidateTogether, which we don't apply inside function expressions. That intuition is confirmed by there being basically no remaining output changes at this point that aren't accounted for, so i'm going to leave as-is and then we can follow-up to remove the PropagatePhiTypes pass and build that into InferTypes