-
Notifications
You must be signed in to change notification settings - Fork 5
Allow delays connected to global inputs and outputs #306
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
Conversation
e4b5884 to
806154e
Compare
| (IRString $ show inSignalId ++ "_" ++ show outSignalId) -- delay edge names are combined | ||
| -- use the input signal id as the delay edge id | ||
| inSignalId | ||
| (findActorByName srcIn) |
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.
This improvement looks great! I think you have written nonDelayInputSignal and nonDelayInputSignal to recursively find the first non delay signal, so it should support a delay actor chain and I wrote a simple example to test it. However, there was an error Actor not found: a_delay2 when I tried running an SDF graph like this:
-- system: s_in -> a_add -> d1 -> d2 -> s_out
I think the error was caused here SDFSchedule:95, when it was converting ForSyDe IR delay edges. It assumed the src and dst of an delay edge should be a non-delay actor so it just used findActorByName to find them, but it failed if there existed an delay chain. So there should also be a recursive way to build delay edges. Should I make a new issue and work on that?
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.
Right, we would need to merge successive delays between regular actors too (and ignore delay-delay connections if the chain is on an input or outout). I think it's more a nice to have since the same model with a single (merged) delay element on the edge should have the same semantic meaning (so we could get the same result by changing the model). Creating the issue would be good though, since we want to document what parts of the compiler is incomplete, and we can work on it if we have time left over.
SamuelMiksits
left a comment
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.
I think this is fine for the most part. I ran all of the models we have now and did not get any errors, and I tried the test suite and it passes. From the code perspective, I looked at it and it looks fine. But someone else might have other opinions or scenarios where it doesn't work
|
I added a few more examples to this PR. |
|
I'm a little confused about how the implementation works. I don't quite understand why adding Perhaps we can talk about it tomorrow in person after the meeting @klarasm. But I can't seem to wrap my head around why the original solution didn't just work once the SDF Scheduler was updated? Nevertheless, the implementation in its current state does work! |
6ef0d2c to
6cf4029
Compare
We can do that.
|
6cf4029 to
03ef1e2
Compare
|
It was a good thing you commented on that, turns out the only thing we actually needed for diff --git a/src/ForSyDeIRToProceduralIR.hs b/src/ForSyDeIRToProceduralIR.hs
index f88c4091b8cc..43974dc59d88 100644
--- a/src/ForSyDeIRToProceduralIR.hs
+++ b/src/ForSyDeIRToProceduralIR.hs
@@ -193,8 +193,8 @@ translateIRConstructor initialContext constructor = case constructor of
++ [EVar $ show functionId]
)
)
- inputStmts = foldl' foldActorSignals [] inputSignals
- outputStmts = foldl' foldActorSignals [] outputSignals
+ inputStmts = foldl' foldActorSignals [] translatedInputSignals
+ outputStmts = foldl' foldActorSignals [] translatedOutputSignals
stmts = reverse inputStmts ++ [actorCallStmt] ++ reverse outputStmts
context1 = initialContext {actors = (actorId, stmts) : actors initialContext}
in context1Then the IO will be handled by the actors as you said. |
|
Hmm. One issue with the approach in this PR is that even if we could produce output immediately we will still try to get input first. But this we can probably put as future work. |
03ef1e2 to
1f4d4bb
Compare
For now just put parentheses around all expressions, regardless of if needed or not. This might be less readable, but the current solution is too simplified. We would additionally need to consider operator precedence and associativeness (left or right).
We have a general solution in CoreIRToProceduralIR now.
Just use the input one. This should make the generated code a bit easier to read. It should also be more efficient in the compilation steps since we're comparing less strings. Note that this also makes an alias for the input edge since we always lookup the alias list in ForSyDeIRToProceduralIR.
1f4d4bb to
2f5290a
Compare
|
I reverted back to just placing parentheses around everything in this PR, though I have another idea in #309. |
I think I address this one in the latest commit. I'm not sure about the formatting on the initial delay tokens on global output, though. |
16015d0 to
262153c
Compare
Erroring out prevents having delay edges on global IO. This shouldn't affect the generated schedule.
Currently, `foldActorSignals` is fed with the unmodified signals, which means it doesn't account for the aliases of delay edges. Change it to use `translatedInputSignals` and `translatedOutputSignals` which do take those into account. This means that IO on delay edges will be taken care of by the regular actor IO.
We want the token rate the first non-delay actor needs so we can get the appropriate number of tokens. Since we will have different names for the buffers, make an alias list in similar way as we do for internal delays.
262153c to
1d4ff63
Compare
Allow delays to be connected to global inputs and outputs.
There are a few parts of this:
foldActorSignalsso it can see the actual buffer id.