<sb0>
whitequark: quite often, a single hardware memory port can be used for both reading and writing. is yosys smart enough not to use two hardware ports when one is sufficient?
<whitequark>
sb0: no, there's no logic for this
<whitequark>
is this for targeting ECP5?
<sb0>
what about ECP5?
<whitequark>
i mean, it doesn't matter for ice40 because ice40 BRAM only has a read only port and a write only port with independent address
<sb0>
okay, well it definitely matters for xilinx devices
<whitequark>
ecp5 does though have true dual port ram
<whitequark>
yes, xilinx is even worse than ecp5
<whitequark>
in terms of bram inference complexity
<ZirconiumX>
So, I've been trying a fairly basic combinational algorithm for flood fill in nMigen, and Yosys warns about logic loops when I try to assign to a Signal using itself as a source
cr1901_modern has quit [Ping timeout: 245 seconds]
cr1901_modern1 has quit [Ping timeout: 244 seconds]
mumptai_ has quit [Quit: Verlassend]
mumptai has joined #m-labs
synaption[m] has joined #m-labs
<vup>
ZirconiumX: yosys is right in the first case
<vup>
maybe it is easier to understand a simpler case:
<vup>
what should
<vup>
m.d.comb += prop.eq(prop << 1)
<vup>
do?
<ZirconiumX>
The value of `prop` is now left shifted by one, so that when you're referring to `prop`, you're referring to the new value, not the old one.
<vup>
but the assignment is combinatorial
<vup>
so as soon as prop is shifted by one it would get shifted again
<vup>
because the output of the shift operation feeds back directly into the input
<vup>
do you get what i mean?
<ZirconiumX>
Yes, but looping the value is pretty obviously a bug and thus not what the user is expecting
<vup>
well thats the definition of combinatorial assignments, they are supposed to work that way
<vup>
like how would the circuit for the combinatorial assigment figure out when to stop?
<ZirconiumX>
...After shifting once?
<ZirconiumX>
Let me put it another way
<ZirconiumX>
What should `m.d.comb += prop1.eq(prop << 1)` do?
<vup>
is assigns prop1 to prop shifted by one
<vup>
and as soon as prop changes prop1 also changes
<ZirconiumX>
I suppose what I'm saying here is that I'm expecting shadowing here, not infinite loops
<vup>
ok, but that is not how signals work in nmigen (or any hdl that i know of)
urkk has quit [Ping timeout: 250 seconds]
<ZirconiumX>
So what you're basically saying is that op-assignment is something that you cannot actually do in nmigen?
<ZirconiumX>
Because nobody in the entire world could possibly want to do op-assignment instead of an infinite loop
<vup>
you can do it perfectly fine, as long as you don't have any combinatorial loops
<ZirconiumX>
And if nmigen renames behind the scenes, you don't get combinational loops
<vup>
yes, but you get order dependence
<vup>
which makes things really ugly
<ZirconiumX>
Frankly if I wanted to deal with the warts of Verilog I'd use Verilog not nMigen
* ZirconiumX
sighs and calms down
<vup>
consider
<vup>
a = Signal(8)
<vup>
c = Signal(8)
<vup>
b = Signal(8)
<vup>
m.d.comb += b.eq(a)
<vup>
m.d.comb += a.eq(a < 1)
<vup>
m.d.comb += c.eq(a)
<vup>
what value do b and c carry?
<vup>
(if you would do the shadowing)
<ZirconiumX>
b has the original value of a, and c is equal to (a < 1)
<vup>
do you not think this could get really confusing?
<ZirconiumX>
...No?
<synaption[m]>
wait
<vup>
hmm, i think a having two potential meanings, depending on where you are in the file quite confusing
<synaption[m]>
I've been following along for a second
<ZirconiumX>
You can't simultanously use both meanings of it, and if you do need both meanings of it, you can use a different variable name
<synaption[m]>
why does c equal a<1
<vup>
ohh whoops, the example should obviously be a << 1
<synaption[m]>
this is HDL right?
<ZirconiumX>
I don't think the operation matters for this scenario
<synaption[m]>
gotcha
<vup>
synaption[m]: yes
<ZirconiumX>
Here's how my mental model of that code goes
<ZirconiumX>
<vup> consider
<ZirconiumX>
<vup> a, a1 = Signal(8), Signal(8)
<ZirconiumX>
<vup> b = Signal(8)
<ZirconiumX>
<vup> c = Signal(8)
<ZirconiumX>
<vup> m.d.comb += b.eq(a)
<ZirconiumX>
<vup> m.d.comb += a1.eq(a < 1)
<ZirconiumX>
<vup> m.d.comb += c.eq(a1)
<vup>
synaption[m]: c being a < 1 is not what (n)migen actually does, we are discussing what would happen if it actually would be that way
<vup>
ZirconiumX: yes
<ZirconiumX>
And thus you can substitute all uses of `c` with `a < 1`
<ZirconiumX>
Kind of like a tree
<vup>
yes
<ZirconiumX>
Can you give me a use for a combinational loop?
<ZirconiumX>
As in, a practical example for where this would be useful behaviour?
<vup>
well there is no use, because it is just not allowed
<ZirconiumX>
Given that nmigen synthesizes it just fine, it seems plenty allowed to me
<vup>
but yosys doesn'=
<vup>
doesn't
<ZirconiumX>
Ang nmigen uses Yosys as a backend, no?
<ZirconiumX>
*And
<vup>
yes
<ZirconiumX>
So, given that the existing behaviour is buggy code, why not forbid it?
<ZirconiumX>
Or use shadowing instead
<vup>
well there are actual use cases
<vup>
like ring oscillators
<ZirconiumX>
<vup> well there is no use, because it is just not allowed
<vup>
yes, i hadn't though of ring oscillators
<vup>
but imo, nmigen should not allow such things by default
<vup>
i don't like the shadowing, because i want use of a signal to actually refer to the same signal
<ZirconiumX>
And I want to not have to type signal0, signal1, signal2, signal3, signal4, when I'm never going to refer to each one individually again
<vup>
sure, but trivially get rid of that (atleast in this case) using a for loop
<vup>
s/but/but you could/
<ZirconiumX>
And that's an example of where nmigen shadows under the hood, no?
<ZirconiumX>
it'll unroll the loop and insert references based on the iteration number
<vup>
well its not really nmigen shadowing, but more python
<vup>
like using a for loop, you'll actually have multiple instances of the Signal, as opposed to the shadowing you're proposing
mumptai has quit [Quit: Verlassend]
<vup>
s/of the Signal/of Signal/
<ZirconiumX>
When something walks like a duck, flies like a duck, and quacks like a duck, I call that thing a duck.
<vup>
i mean the difference is using a for loop you have to manually code the shadowing and it doesn't do it automagically
<ZirconiumX>
I'd say it does happen automagically, because I don't consciously think of it instantiating new variables, I just think of it as shadowing the old one
<vup>
but using a for loop you'll have to manually instantiate a Signal
<vup>
like you'll have to write something like this:
<vup>
tmp = Signal(8)
<vup>
m.d.comb += tmp.eq(prop)
<vup>
m.d.comb += tmp_next.eq(tmp <. 1)
<vup>
tmp_next = Signal(8)
<vup>
for _ in range(4)
<vup>
tmp = tmp_next
<vup>
(not valid python, but you get what i mean)
<ZirconiumX>
vup, do you write all your Python code using unique names for each variable? Or do you just use compound assignment to existing ones?
<lkcl>
ZirconiumX: "Frankly if I wanted to deal with the warts of Verilog..."
<lkcl>
it's just how hardware works.
<lkcl>
nothing to do with what HDL you're using.
<vup>
ZirconiumX: i don't, but in my mind multiple lines in python are more comparable to synchronous statements
<ZirconiumX>
...No? That's not how hardware has to work at all
<lkcl>
you could take some wires - i mean actual copper wires - and if you create a short-circuit, you create a short-circuit, that's the end of the battery!
<ZirconiumX>
And that's why kids at primary school get told not to short circuit batteries, because it's dangerous, right?
<lkcl>
m.d.comb += prop.eq(prop << 1)
<lkcl>
connects all the wires of prop to all the wires to the LEFT of each wire in prop
<lkcl>
total utter chip-melting suite of short-circuits.
<lkcl>
melt, melt.
<lkcl>
prop[0] becomes a short-circuit to prop[1]
<ZirconiumX>
Yes, I get it now
<lkcl>
prop[1] becomes a short-circuit to prop[2]
<lkcl>
etc. etc.
<ZirconiumX>
As has been pointed out repeatedly
<lkcl>
the only way to deal with that is to have a flip-flop in between
<lkcl>
gated by a clock
<lkcl>
and you do that with m.d.sync
<ZirconiumX>
Which is synchronous, yes
<ZirconiumX>
I'm not looking for synchronous logic here
<lkcl>
ok then what you can do is, assign it to a NEW variable