Skip to content

[compiler] comments and todos #33376

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

Closed
wants to merge 18 commits into from
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {inferMutableLifetimes} from './InferMutableLifetimes';
import {inferMutableRangesForAlias} from './InferMutableRangesForAlias';
import {inferMutableRangesForComutation} from './InferMutableRangesForComutation';
import {inferTryCatchAliases} from './InferTryCatchAliases';
import {printIdentifier, printMutableRange} from '../HIR/PrintHIR';
import {printIdentifier} from '../HIR/PrintHIR';

export function inferMutableRanges(ir: HIRFunction): DisjointSet<Identifier> {
// Infer mutable ranges for non fields
Expand Down Expand Up @@ -105,7 +105,8 @@ export function debugAliases(aliases: DisjointSet<Identifier>): void {
.buildSets()
.map(set =>
[...set].map(
ident => printIdentifier(ident) + printMutableRange(ident),
ident =>
`${printIdentifier(ident)}:${ident.mutableRange.start}:${ident.mutableRange.end}`,
),
),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ function computeSignatureForInstruction(
value.args,
)
: null;
if (signatureEffects != null && signature?.aliasing != null) {
if (signatureEffects != null) {
effects.push(...signatureEffects);
} else if (signature != null) {
effects.push(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import DisjointSet from '../Utils/DisjointSet';
import {assertExhaustive} from '../Utils/utils';
import {inferMutableRangesForAlias} from './InferMutableRangesForAlias';

/**
* Infers mutable ranges for all values.
*/
export function inferMutationAliasingRanges(fn: HIRFunction): void {
/**
* Part 1
Expand Down Expand Up @@ -64,13 +67,34 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
}
}
}
/**
* TODO: this will incorrectly mark values as mutated in the following case:
* 1. Create value x
* 2. Create value y
* 3. Transitively mutate y
* 4. Capture x -> y
*
* We will put these all into one alias set in captures, and then InferMutableRangesForAlias
* will look at all identifiers in the set that start before the mutation. Thus it will see
* that x is created before the mutation, and consider it mutated. But the capture doesn't
* occur until after the mutation!
*
* The fix is to use a graph to precisely describe what is captured where at each instruction,
* so that on a transitive mutation we can iterate all the captured values and mark them.
*
* This then needs a fixpoint: although we would track captures for phi operands, the operands'
* own capture values won't be populated until we do a fixpoint.
*/
inferMutableRangesForAlias(fn, captures);

/**
* Part 2
* Infer ranges for local (non-transitive) mutations. We build a disjoint set
* that only tracks direct value aliasing, and look only at local mutations
* to extend ranges
*
* TODO: similar design to the above todo for captures, use a directed graph instead of disjoint union,
* with fixpoint.
*/
const aliases = new DisjointSet<Identifier>();
for (const block of fn.body.blocks.values()) {
Expand Down
Loading