-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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] Add fallthrough to branch terminal #30814
Changes from all commits
1846220
01b9791
fed48b4
00d44be
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 |
---|---|---|
|
@@ -42,6 +42,7 @@ type IdentifierSidemap = { | |
react: Set<IdentifierId>; | ||
maybeDepsLists: Map<IdentifierId, Array<Place>>; | ||
maybeDeps: Map<IdentifierId, ManualMemoDependency>; | ||
optionals: Set<IdentifierId>; | ||
}; | ||
|
||
/** | ||
|
@@ -52,6 +53,7 @@ type IdentifierSidemap = { | |
export function collectMaybeMemoDependencies( | ||
value: InstructionValue, | ||
maybeDeps: Map<IdentifierId, ManualMemoDependency>, | ||
optional: boolean, | ||
): ManualMemoDependency | null { | ||
switch (value.kind) { | ||
case 'LoadGlobal': { | ||
|
@@ -69,7 +71,7 @@ export function collectMaybeMemoDependencies( | |
return { | ||
root: object.root, | ||
// TODO: determine if the access is optional | ||
path: [...object.path, {property: value.property, optional: false}], | ||
path: [...object.path, {property: value.property, optional}], | ||
}; | ||
} | ||
break; | ||
|
@@ -162,7 +164,11 @@ function collectTemporaries( | |
break; | ||
} | ||
} | ||
const maybeDep = collectMaybeMemoDependencies(value, sidemap.maybeDeps); | ||
const maybeDep = collectMaybeMemoDependencies( | ||
value, | ||
sidemap.maybeDeps, | ||
sidemap.optionals.has(lvalue.identifier.id), | ||
); | ||
// We don't expect named lvalues during this pass (unlike ValidatePreservingManualMemo) | ||
if (maybeDep != null) { | ||
sidemap.maybeDeps.set(lvalue.identifier.id, maybeDep); | ||
|
@@ -338,12 +344,14 @@ export function dropManualMemoization(func: HIRFunction): void { | |
func.env.config.validatePreserveExistingMemoizationGuarantees || | ||
func.env.config.validateNoSetStateInRender || | ||
func.env.config.enablePreserveExistingMemoizationGuarantees; | ||
const optionals = findOptionalPlaces(func); | ||
const sidemap: IdentifierSidemap = { | ||
functions: new Map(), | ||
manualMemos: new Map(), | ||
react: new Set(), | ||
maybeDeps: new Map(), | ||
maybeDepsLists: new Map(), | ||
optionals, | ||
}; | ||
let nextManualMemoId = 0; | ||
|
||
|
@@ -476,3 +484,46 @@ export function dropManualMemoization(func: HIRFunction): void { | |
} | ||
} | ||
} | ||
|
||
function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> { | ||
const optionals = new Set<IdentifierId>(); | ||
for (const [, block] of fn.body.blocks) { | ||
if (block.terminal.kind === 'optional') { | ||
const optionalTerminal = block.terminal; | ||
let testBlock = fn.body.blocks.get(block.terminal.test)!; | ||
loop: while (true) { | ||
const terminal = testBlock.terminal; | ||
switch (terminal.kind) { | ||
case 'branch': { | ||
if (terminal.fallthrough === optionalTerminal.fallthrough) { | ||
// found it | ||
const consequent = fn.body.blocks.get(terminal.consequent)!; | ||
const last = consequent.instructions.at(-1); | ||
if (last !== undefined && last.value.kind === 'StoreLocal') { | ||
optionals.add(last.value.value.identifier.id); | ||
} | ||
break loop; | ||
} else { | ||
testBlock = fn.body.blocks.get(terminal.fallthrough)!; | ||
} | ||
break; | ||
} | ||
case 'optional': | ||
case 'logical': | ||
case 'sequence': | ||
case 'ternary': { | ||
Comment on lines
+512
to
+514
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. Ah I see, I'm guessing that this is to support something like 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. yup exactly |
||
testBlock = fn.body.blocks.get(terminal.fallthrough)!; | ||
break; | ||
} | ||
default: { | ||
CompilerError.invariant(false, { | ||
reason: `Unexpected terminal in optional`, | ||
loc: terminal.loc, | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
return optionals; | ||
} |
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.
This feels slightly weird since DropManualMemo runs before we enter ssa, so both consequent (the optional read) and alternate (
undefined
) write to the same identifier ID. There is also no phi join, so reads to the OptionalExpression rvalue is also guaranteed to use the same identifier ID (and this works)(I'm guessing that passes after SSA will need to track phis as well -- reading the rest of the stack now)
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.
Yeah I looked at using the phi to figure out which values is optional. First, as you noted we aren't in SSA so we don't have phis yet. Second, the phis end up in weird places for nested optionals, because with nested optionals the alternate actually goes to the outermost fallthrough. So we just look at where the consequent assigns to in order to figure out what the optional value was.