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

Add condition based MethodFilter #504

Merged
merged 11 commits into from
Dec 3, 2023
Merged
Show file tree
Hide file tree
Changes from 9 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
12 changes: 8 additions & 4 deletions test/transactions/test_transaction_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,23 +466,27 @@ def target_mock(self, data):
def cmeth_mock(self, data):
return {"data": data % 2}

def test_method_filter_with_methods(self):
@parameterized.expand([(True,), (False,)])
def test_method_filter_with_methods(self, use_condition):
self.initialize()
self.cmeth = TestbenchIO(Adapter(i=self.layout, o=data_layout(1)))
self.tc = SimpleTestCircuit(MethodFilter(self.target.adapter.iface, self.cmeth.adapter.iface))
self.tc = SimpleTestCircuit(
MethodFilter(self.target.adapter.iface, self.cmeth.adapter.iface, use_condition=use_condition)
)
m = ModuleConnector(test_circuit=self.tc, target=self.target, cmeth=self.cmeth)
with self.run_simulation(m) as sim:
sim.add_sync_process(self.source)
sim.add_sync_process(self.target_mock)
sim.add_sync_process(self.cmeth_mock)

def test_method_filter(self):
@parameterized.expand([(True,), (False,)])
def test_method_filter(self, use_condition):
self.initialize()

def condition(_, v):
return v[0]

self.tc = SimpleTestCircuit(MethodFilter(self.target.adapter.iface, condition))
self.tc = SimpleTestCircuit(MethodFilter(self.target.adapter.iface, condition, use_condition=use_condition))
m = ModuleConnector(test_circuit=self.tc, target=self.target)
with self.run_simulation(m) as sim:
sim.add_sync_process(self.source)
Expand Down
33 changes: 26 additions & 7 deletions transactron/lib/transformers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from collections.abc import Callable
from transactron.utils import ValueLike, assign, AssignType, ModuleLike
from .connectors import Forwarder, ManyToOneConnectTrans, ConnectTrans
from .simultaneous import condition

__all__ = [
"Transformer",
Expand Down Expand Up @@ -109,9 +110,10 @@ class MethodFilter(Transformer, Elaboratable):
parameters, a module and the input `Record` of the method. Non-zero
return value is interpreted as true. Alternatively to using a function,
a `Method` can be passed as a condition.

Caveat: because of the limitations of transaction scheduling, the target
method is locked for usage even if it is not called.
By default, the target method is locked for use even if it is not called.
If this is not the desired effect, set `use_condition` to True, but this will
cause that the provided method will be `single_caller` and all other `condition`
drawbacks will be in place (e.g. risk of exponential complexity).

Attributes
----------
Expand All @@ -120,7 +122,11 @@ class MethodFilter(Transformer, Elaboratable):
"""

def __init__(
self, target: Method, condition: Callable[[TModule, Record], ValueLike], default: Optional[RecordDict] = None
self,
target: Method,
condition: Callable[[TModule, Record], ValueLike],
default: Optional[RecordDict] = None,
use_condition: bool = False,
):
"""
Parameters
Expand All @@ -133,12 +139,16 @@ def __init__(
default: Value or dict, optional
The default value returned from the filtered method when the condition
is false. If omitted, zero is returned.
use_condition : bool
Instead of `m.If` use simultaneus `condition` which allow to execute
this filter if the condition is False and target is not ready.
"""
if default is None:
default = Record.like(target.data_out)

self.target = target
self.method = Method.like(target)
self.use_condition = use_condition
self.method = Method(i=target.data_in.layout, o=target.data_out.layout, single_caller=self.use_condition)
self.condition = condition
self.default = default

Expand All @@ -150,8 +160,17 @@ def elaborate(self, platform):

@def_method(m, self.method)
def _(arg):
with m.If(self.condition(m, arg)):
m.d.comb += ret.eq(self.target(m, arg))
if self.use_condition:
cond = Signal()
m.d.top_comb += cond.eq(self.condition(m, arg))
with condition(m, nonblocking=False) as branch:
Copy link
Member

Choose a reason for hiding this comment

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

The conditions are mutually exclusive - maybe priority=False should be used?

Or remove the second branch and use nonblocking=True, as @piotro888 suggested. Right now you use both exclusivity and transaction priorities - do you see any advantage of this, or is this accidental?

Copy link
Member

Choose a reason for hiding this comment

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

I'm waiting with merge until this is resolved.

Copy link
Contributor Author

@lekcyjna123 lekcyjna123 Dec 3, 2023

Choose a reason for hiding this comment

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

We can not use nonblocking=True here. The condition class doesn't use two state logic. We have:

  • True and ready
  • False and ready
  • Not ready

If the cond=1 and target is not ready, then with nonblocking=True the transaction will execute and so the whole method will execute, which will be wrong because the data instead of being forwarded will be dropped.

I added priority=False, this shouldn't impact the logic.

with branch(cond):
m.d.comb += ret.eq(self.target(m, arg))
with branch(~cond):
pass
else:
with m.If(self.condition(m, arg)):
m.d.comb += ret.eq(self.target(m, arg))
return ret

return m
Expand Down