Skip to content
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

Dependency finding #704

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from
Draft

Conversation

a-alveyblanc
Copy link
Contributor

New draft PR based on PR 683. Major change is that the HappensAfter data structure used to represent statement-level dependency relations has been implemented into the InstructionBase data structure.

@inducer inducer changed the title New branch for dependency finding Dependency finding Nov 14, 2022
@inducer inducer mentioned this pull request Nov 14, 2022
variable_name: Optional[str]
instances_rel: Optional[Map]

class AccessMapMapper(WalkMapper):
Copy link
Owner

@inducer inducer Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this a CombineMapper? (In that case, you'd need to define combine)

self._var_names = var_names

from collections import defaultdict
self.access_maps = defaultdict(lambda:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document what the keys/values of these mappings are.


super.__init__()

def map_subscript(self, expr, inames, insn_id):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think about type annotation

try:
access_map = get_access_map(domain, subscript)
except UnableToDetermineAccessRangeError:
return
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the conservative approach?

except UnableToDetermineAccessRangeError:
return

if self.access_maps[insn_id][arg_name][inames] is None:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the else case?

}

new_insns = []
for insn in knl.instructions:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think about transitive dependencies

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git grep -i transitive

# return the kernel with the new instructions
return knl.copy(instructions=new_insns)

def add_lexicographic_happens_after(knl: LoopKernel) -> None:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lexicographic happens before is the "coarsest" ordering, and the above would be the refinement of that based on data dependencies.

Comment on lines +197 to +198
assert shared_inames_order_after == shared_inames_order_before
shared_inames_order = shared_inames_order_after
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a big unresolved question here, in terms of what nesting order we should use for the shared inames. Right now, this uses the axis order in the domains, however we also already do have LoopKernel.loop_priority to indicate nesting order during code generation). If nothing else, this should make sure that the order produced is consistent with that. But there's also the option of using/introducing a different mechanism entirely for this.

@kaushikcfd, got an opinion?

Comment on lines 78 to 79
def map_linear_subscript(self, expr, insn):
self.rec(expr.index, insn)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably error.


amf = AccessMapFinder(knl)
for insn in knl.instructions:
amf(insn.assignee, insn)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Some instructions (specifically CallInstructions) have more than one assignee.

def get_map(self, insn_id: str, variable_name: str) -> isl.Map:
try:
return self.access_maps[insn_id][variable_name]
except KeyError:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this KeyError ever happen?


def __init__(self, knl: LoopKernel) -> None:
self.kernel = knl
self.access_maps = defaultdict(lambda: defaultdict(lambda: None))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe avoid defaultdicts?

domain, subscript, self.kernel.assumptions
)

if self.access_maps[insn.id][arg_name]:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.access_maps[insn.id][arg_name]:
if self.access_maps[insn.id][arg_name] is not None:

def map_reduction(self, expr, insn):
return WalkMapper.map_reduction(self, expr, insn)

def map_type_cast(self, expr, inames):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def map_type_cast(self, expr, inames):
def map_type_cast(self, expr, insn):

Comment on lines 139 to 140
accessed_by = reader_map.get(variable, set()) | \
writer_map.get(variable, set())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you still avoiding read-after-read?

…ndency finding in narrow_dependencies [skip ci]
Comment on lines +1192 to 1194
happens_after=(
red_realize_ctx.surrounding_depends_on
| frozenset([init_id, init_neutral_id])),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code suggests that happens_after is still a set?


@for_each_kernel
def narrow_dependencies(knl: LoopKernel) -> LoopKernel:
"""Attempt to relax a strict (lexical) ordering between statements in a
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lexicographic or lexical?


@for_each_kernel
def narrow_dependencies(knl: LoopKernel) -> LoopKernel:
"""Attempt to relax a strict (lexical) ordering between statements in a
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not safe to assume that the input ordering is lexicographic.

statements.
"""

assert isinstance(insn.id, str) # stop complaints
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns this into an actual type annotation in InstructionBase.

t_sort = compute_topological_order(deps_dag)

for insn_id in t_sort:
transitive_deps[insn_id] = reduce(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transitive_deps can't help but be quadratic in the size of the program.

Comment on lines 216 to 218
lex_map = insn.happens_after[dependency].instances_rel
else:
lex_map = dep_map.lex_lt_map(dep_map)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the fine-grain/instance-level dependency info that's already in the kernel at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants