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

Error comparing Mem reads #57

Closed
jemcmahan13 opened this issue Mar 11, 2015 · 12 comments
Closed

Error comparing Mem reads #57

jemcmahan13 opened this issue Mar 11, 2015 · 12 comments

Comments

@jemcmahan13
Copy link
Contributor

Right now a read port returns a _MemIndexed. Normal operations are not defined on this class, so attempting to compare two reads returns pyrtl.core.PyrtlError: error, attempt to covert wirevector to compile-time boolean, and attempting a logical operations returns an unsupported operand types error.

import sys
sys.path.append("..")
from pyrtl import *


regfile = MemBlock(8, 3)
# Both of the following lines will fail
regfile[0] == regfile[1]
regfile[0] & regfile[1]
@timsherwood
Copy link
Contributor

I was working on this -- the basic problem is that a _MemIndexed will be converted to a WireVector when it is combined with another WireVector or assigned to a WireVector ( as_wires() does the conversion ). But when two _MemIndexed are combined it does not know what to do. 3 options that I see:

  1. Implement the full set of WireVector operations converting from _MemIndexed. This results in a lot of boilerplate code and I worry about keeping it in sync with WireVectors

  2. Use multiple-inheritance to inherit from WireVector. This is kind of a hack, but would make the code shorter and more consistent.

  3. Define a _WireVectorInterface from which WireVector and _MemIndexed can both be derived. This is a reasonably serious change to WireVector but should not change any of the other code as far as I can think.

@timsherwood
Copy link
Contributor

Added a test but commented out for with """ for now.

@Gamrix
Copy link
Contributor

Gamrix commented Mar 14, 2015

@timsherwood
Copy link
Contributor

Neat. Does the same restriction hold for namedtuples? Those are implemented in the collections library and are not technically builtin.

@Gamrix
Copy link
Contributor

Gamrix commented Mar 14, 2015

According to the docstring in the collections source code, they actually "subclass" tuple.

@timsherwood
Copy link
Contributor

Maybe that is why when I tried it acted all weird. Any thoughts on 1 or 3?

@Gamrix
Copy link
Contributor

Gamrix commented Mar 14, 2015

actually
a = collections.namedtuple("a","b")
In[5]: isinstance(a,tuple)
Out[5]: False
In[6]: isinstance(a,object)
Out[6]: True
In[19]: isinstance(a,type)
Out[19]: True

In[10]: b = tuple([1,2])
In[11]: isinstance(b,tuple)
Out[11]: True
In[12]: isinstance(b,object)
Out[12]: True
In[21]: isinstance(b,type)
Out[21]: False

Now I don't know what to believe

@Gamrix
Copy link
Contributor

Gamrix commented Mar 14, 2015

I think that 3 is the better choice, and I think it won't be as hard as you anticipate. For ROMs, I just lifted the read related calls from memblock and put them into _MemReadBase. The hardest part about the whole thing was tracking down the few places where there was a call to isinstance(xxx, Memblock) where I wanted the code to apply to ROMs as well.

@timsherwood
Copy link
Contributor

works for me. Once you have ROMs committed I will work on that -- I think there is a good chance of conflicts if I don't wait.

@jemcmahan13
Copy link
Contributor Author

Issue has resurfaced where a _MemIndexed is lacking attributes a WireVector has (in this case, block).

from pyrtl import *
RF = MemBlock(8, 3)
val1 = RF[0]
test1 = concat(0, val1)

@jemcmahan13
Copy link
Contributor Author

This is not a critical fix, by the way; for now I can just pre-declare read-values as WireVectors and assign the result of the port.

@timsherwood
Copy link
Contributor

fixed (it could have showed up a couple of places, such as in a mux as well) 11577a8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants