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

Conversation

lekcyjna123
Copy link
Contributor

This MR is the first fragment of #395 to port to the trunk. Here I extend MethodFilter with possibility to use condition under the hood to ignore situations, where method is not ready and condition is false. This has the caveats that resulting filter is single_caller so both versions should coexistence.

@tilk tilk added the enhancement New feature or request label Nov 5, 2023
@lekcyjna123
Copy link
Contributor Author

A remark to remember is that the usage of validate_arguments is not possible in MethodFilter, because this feature cause blocking of method, if the condition is not fulfilled, what cause the blocking of the calling transaction.

@tilk tilk added the review needed This PR needs reviews very much. label Nov 20, 2023
Copy link
Member

@piotro888 piotro888 left a comment

Choose a reason for hiding this comment

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

Approved, but maybe if it doesn't cause any logic penalty this:

               with condition(m, nonblocking=True) as branch:
                    with branch(cond):
                        m.d.comb += ret.eq(self.target(m, arg))

would be cleaner? (nonblocking=True to automatically generate empty default condition - equivalent to ~cond)

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.

Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

Address the review comment.

@tilk tilk changed the title Add condition based MetodFilter Add condition based MethodFilter Dec 3, 2023
@tilk tilk merged commit a053bb5 into kuznia-rdzeni:master Dec 3, 2023
8 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 3, 2023
tilk pushed a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review needed This PR needs reviews very much.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants