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

Python logical operator 'and' silently dropped #137

Open
tdaede opened this issue Sep 27, 2018 · 18 comments
Open

Python logical operator 'and' silently dropped #137

tdaede opened this issue Sep 27, 2018 · 18 comments

Comments

@tdaede
Copy link

tdaede commented Sep 27, 2018

The following migen snippet:

               If((self.addr == 0x18a) and (self.joy_enabled == 1),

produces the following Verilog:

                if ((addr_1 == 9'd394)) begin
@sbourdeauducq
Copy link
Member

Yes, you cannot use and - it cannot be redefined in Python.

@tdaede
Copy link
Author

tdaede commented Sep 27, 2018

Is there any way to make it a hard error?

@sbourdeauducq
Copy link
Member

Actually it should raise TypeError("Attempted to convert Migen value to boolean") (see fhdl/structure.py). Can you find out why that does not happen?

@whitequark whitequark added the bug label Nov 10, 2018
@whitequark
Copy link
Contributor

@sbourdeauducq Because of the special case in __bool__:

        # Special case: Constants and Signals are part of a set or used as
        # dictionary keys, and Python needs to check for equality.
        if isinstance(self, _Operator) and self.op == "==":
            a, b = self.operands
            if isinstance(a, Constant) and isinstance(b, Constant):
                return a.value == b.value
            if isinstance(a, Signal) and isinstance(b, Signal):
                return a is b
            if (isinstance(a, Constant) and isinstance(b, Signal)
                    or isinstance(a, Signal) and isinstance(b, Constant)):
                return False

I don't understand why it lives in __bool__ though.

@sbourdeauducq
Copy link
Member

Where should it be instead?

@whitequark
Copy link
Contributor

@sbourdeauducq Is it realistic to make only Signal and Constant hashable in the first place? AFAICT Migen does not rely on other values being hashable, but I might be wrong.

@sbourdeauducq
Copy link
Member

Maybe, can you test it (including with ARTIQ)?

@whitequark
Copy link
Contributor

OK

@xobs
Copy link
Contributor

xobs commented Apr 18, 2019

I also just ran into this problem:

class OrderTest(Module):
    def __init__(self):
        counter = Signal(8)
        sig = Signal()
        self.sync += [
            counter.eq(counter + 1),
            If((counter == 1) or (counter == 2) or (counter == 3),
                sig.eq(1)
            )
        ]

print(str(verilog.convert(OrderTest())))

This results in:

...
      if ((counter == 2'd3)) begin
                sig <= 1'd1;
      end
...

@whitequark
Copy link
Contributor

FYI this (and many other issues) are fixed in https://github.com/m-labs/nmigen/. But nMigen is not yet quite ready.

@whitequark
Copy link
Contributor

Triage: fixed in nMigen.

@DurandA
Copy link
Contributor

DurandA commented Oct 29, 2019

@sbourdeauducq @whitequark What is the correct way of doing logical AND or logical OR in Migen then? and_(a, b)? I couldn't find any example with something similar to _Operator("and", [a, b]) or _Operator("||", [a, b]) in the repository.

@jordens
Copy link
Member

jordens commented Oct 29, 2019

(x == 2) & (y > 9)

@DurandA
Copy link
Contributor

DurandA commented Oct 29, 2019

(x == 2) & (y > 9)

This is the bitwise operator, which does not behave like the logical operator.

@jordens
Copy link
Member

jordens commented Oct 29, 2019

It does. For one bit expressions they are the same.

@whitequark
Copy link
Contributor

whitequark commented Oct 30, 2019

In nMigen you can use x.bool() & y.bool() to get an equivalent of the logical AND that works regardless of the width of x and y.

@DurandA
Copy link
Contributor

DurandA commented Oct 30, 2019

Thanks for your answers! Is there a bool() equivalent in Migen or should we use something like reduce(or_, a)?

@jordens
Copy link
Member

jordens commented Oct 30, 2019

a != 0

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

No branches or pull requests

6 participants