-
Notifications
You must be signed in to change notification settings - Fork 14
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
Priority ordering proxy #355
Conversation
coreblocks/transactions/lib.py
Outdated
with Transaction().body(m): | ||
with condition(m, priority=True) as branch: | ||
for k in range(len(self.m_unordered), 0, -1): | ||
for comb_un in itertools.combinations(self._m_unordered_read, k): | ||
with branch(1): # sic! | ||
branch_trans = TransactionBase.get() | ||
for un, ord in zip(comb_un, self.m_ordered): | ||
trans_mod = ConnectTrans(un, ord) | ||
trans_mod.t_elaborate() | ||
m.submodules += trans_mod | ||
branch_trans.simultaneous(trans_mod.transaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from just creating multiple transactions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand things correctly, what this whole thing does is:
- It gender-changes the input, so that you provide methods. instead of taking methods. (We didn't do this consequently before, but I'd prefer to externalize gender-changing if possible. In other words, first create an Elaboratable which has the most natural interface for it, and then provide gender-changed variants if possible.)
- For each subsequence of the input methods, it creates a transaction which connects N first methods from
m_ordered
to the methods from the subsequence. Also, these transactions are ordered.
My remarks are:
- The use of simultaneous transactions is completely unneeded for the point 2 above. You are creating a lot of work for the transaction manager, even though you could easily express this directly - and it would probably be more readable, too. Edit: I kind of see your point. What you really want is something kind of like
condition
, but without the base transaction. Maybe it's possible to modifycondition
to achieve that? Needs to be investigated. This thing imposes too much ordering. If I understand correctly, you actually want the largest subsequences to be ordered before the smallest. This is a weaker, non-linear ordering.Edit: The ordering of the subsequences is probably important for you, too, I'm retracting this remark.- Creating ConnectTrans just to mess with its insides later is very ugly. I understand you wanted to avoid repeating these four lines inside
ConnectTrans
, but this definitely is a very convoluted way to do this. An alternative would be, for example, to create a function which does this, and use it both inConnectTrans
and here. (We currently don't have a simple way to compose transactions like you would want. The way you proposed is weirdly convoluted and certainly not the way to go.)
Moved to #395 |
Here is a PR which create a method ordering proxy. It will be next used in MultiportFifo to order calls on transactron level (instead of ordering on coreblock logic level). Both
with condition
andConnect
were useful in this PR.This depends on: #304
#347