Skip to content

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Oct 7, 2022

TODOs:

  • add to MemAccess, Sync

Please squash

@matthiasdiener matthiasdiener changed the title Create WithTag add WithTag Oct 7, 2022
@matthiasdiener
Copy link
Collaborator Author

Is this going in the direction you had in mind @inducer ?

Comment on lines 932 to 935
opmap = self.rec(expr.expr)
for op in opmap.count_map:
op.tags = expr.tags
return opmap
Copy link
Owner

Choose a reason for hiding this comment

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

Would overwrite tags of subexpressions that already have tags.

self.rec(expr.expr, expr.tags)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is 50adbc4 what you had in mind?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, generally.

@matthiasdiener matthiasdiener changed the title add WithTag add TaggedExpression Oct 18, 2022

map_bitwise_xor = map_bitwise_or
map_bitwise_and = map_bitwise_or

def map_if(self, expr):
def map_if(self, expr, *args):
Copy link
Owner

Choose a reason for hiding this comment

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

Why switch to *args here?

Copy link
Collaborator Author

@matthiasdiener matthiasdiener Nov 9, 2022

Choose a reason for hiding this comment

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

In 0bbbaec, I changed all occurrences to tags (with a default argument). Does this look reasonable?

@inducer
Copy link
Owner

inducer commented Oct 18, 2022

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@matthiasdiener matthiasdiener marked this pull request as ready for review November 9, 2022 23:32
@matthiasdiener
Copy link
Collaborator Author

This is ready for a first look @inducer.

@@ -916,14 +931,17 @@ def __init__(self, knl, callables_table, kernel_rec,
def combine(self, values):
return sum(values)

def map_constant(self, expr):
def map_constant(self, expr, tags=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Why the pervasive tags=None? The default risks losing the tag as you traverse. IMO, there should not be a default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it in df7ed9b to not use a default argument anymore.

else:
return self.new_zero_poly_map()

map_product = map_sum

def map_comparison(self, expr):
return self.rec(expr.left)+self.rec(expr.right)
def map_comparison(self, expr, *args):
Copy link
Owner

Choose a reason for hiding this comment

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

Why have *args instead of tags here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are hopefully all fixed in df7ed9b

matthiasdiener and others added 2 commits January 13, 2025 12:58
Co-authored-by: Andreas Klöckner <inform@tiker.net>
@matthiasdiener
Copy link
Collaborator Author

I think this is ready for another review.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! This is quite close, only a few minor wrinkles to finish up.

count_granularity=local_mem_count_granularity,
kernel_name=self.knl.name): self.one})

elif (isinstance(array, TemporaryVariable) and (
Copy link
Owner

Choose a reason for hiding this comment

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

Should this also apply to ArrayArg? / Where do those get counted if not?

Copy link
Collaborator Author

@matthiasdiener matthiasdiener Feb 20, 2025

Choose a reason for hiding this comment

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

Doesn't it already apply to ArrayArg (see 2 lines below)?

@property
def mtype(self) -> str:
from warnings import warn
warn("MemAccess.mtype is deprecated and will stop working in 2024. "
Copy link
Owner

Choose a reason for hiding this comment

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

Bump deadlines to 2026? (here and elsewhere)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 0b9d8a4

return f"Sync({self.kind}, {self.kernel_name})"
# Overridden for conciseness
return "Sync({}, {}, {})".format(
self.sync_kind, repr(self.kernel_name), self.tags)

# }}}


# {{{ CounterBase

class CounterBase(CombineMapper):
Copy link
Owner

Choose a reason for hiding this comment

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

Likely missing type arguments.

Comment on lines 792 to 803
.. attribute:: tags

A :class:`frozenset` of subclasses of :class:`pytools.tag.Tag` used to
provide metadata on this expression.

.. attribute:: expr

An expression to which :attr:`tags` are attached.
"""

tags: frozenset[Tag]
expr: Expression
Copy link
Owner

Choose a reason for hiding this comment

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

Have this use autoattribute, move docstrings after declaration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 6661eff



@dataclass(frozen=True, eq=True)
class MemAccess:
"""A descriptor for a type of memory access.
Copy link
Owner

Choose a reason for hiding this comment

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

Move to autoattribute and in-line docstrings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 6959904


# }}}


# {{{ Op descriptor

class Op(ImmutableRecord):
class OpType(Enum):
Copy link
Owner

Choose a reason for hiding this comment

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

Move to autoattribute and in-line docstrings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure - does adding in-line docstrings add a lot of information to these values? Also, with autoattribute, sphinx shows the integer values of these constants, which I think isn't very meaningful.

image

Copy link
Owner

Choose a reason for hiding this comment

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

The semantics of tagging in connection with counting need to be explained somewhere in the documentation.

@matthiasdiener
Copy link
Collaborator Author

matthiasdiener commented Jun 10, 2025

I can't get this to work with pytato:

import numpy as np

import pytato as pt
import loopy as lp

from pytools.tag import Tag

class FooTag(Tag):
    pass

a = pt.make_placeholder(name="a", shape=(10, 10), dtype=np.float64)

a2 = a + a
result = pt.make_dict_of_named_arrays({"aout": a2.tagged(FooTag())})

prg = pt.generate_loopy(result)

op_map = lp.get_op_map(prg.program, subgroup_size=32)

print("UNFILTERED", op_map)

print("FILTERED", op_map.filter_by(tags=[frozenset((FooTag(),))]))

print(prg.program)
$ python examples/demo.py
/Users/mdiener/Work/emirge/loopy/loopy/statistics.py:1801: LoopyWarning: in kernel _pt_kernel: get_insn_count: when counting instruction aout_store with count_granularity=CountGranularity.SUBGROUP, using upper bound for work-group size (1 work-items) to compute sub-groups per work-group. When multiple device programs present, actual sub-group count may be lower. (add 'insn_count_subgroups_upper_bound' to silenced_warnings kernel attribute to disable)
  warn_with_kernel(knl, "insn_count_subgroups_upper_bound",
UNFILTERED Op(np:dtype('float64'), OpType.ADD, CountGranularity.SUBGROUP, "_pt_kernel", frozenset()): { 100 }
FILTERED
---------------------------------------------------------------------------
KERNEL: _pt_kernel
---------------------------------------------------------------------------
ARGUMENTS:
a: type: np:dtype('float64'), shape: (10, 10), dim_tags: (N1:stride:10, N0:stride:1), offset: <class 'loopy.typing.auto'> in aspace: global
aout: type: np:dtype('float64'), shape: (10, 10), dim_tags: (N1:stride:10, N0:stride:1), tags: {FooTag()} out aspace: global
---------------------------------------------------------------------------
DOMAINS:
{  :  }
{ [aout_dim0, aout_dim1] : 0 <= aout_dim0 <= 9 and 0 <= aout_dim1 <= 9 }
---------------------------------------------------------------------------
INAME TAGS:
aout_dim0: None
aout_dim1: None
---------------------------------------------------------------------------
INSTRUCTIONS:
for aout_dim0, aout_dim1
    aout[aout_dim0, aout_dim1] = a[aout_dim0, aout_dim1] + a[aout_dim0, aout_dim1]  {id=aout_store}
end aout_dim0, aout_dim1
---------------------------------------------------------------------------

Pytato does not create TaggedExpressions itself, which is likely the reason they don't show up in the Op class.

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