m-labs / migen

A Python toolbox for building complex digital hardware
https://m-labs.hk/migen
Other
1.21k stars 209 forks source link

Python logical operator 'and' silently dropped #137

Open tdaede opened 6 years ago

tdaede commented 6 years ago

The following migen snippet:

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

produces the following Verilog:

                if ((addr_1 == 9'd394)) begin
sbourdeauducq commented 6 years ago

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

tdaede commented 6 years ago

Is there any way to make it a hard error?

sbourdeauducq commented 6 years ago

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 commented 5 years ago

@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 commented 5 years ago

Where should it be instead?

whitequark commented 5 years ago

@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 commented 5 years ago

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

whitequark commented 5 years ago

OK

xobs commented 5 years ago

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 commented 5 years ago

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

whitequark commented 5 years ago

Triage: fixed in nMigen.

DurandA commented 4 years ago

@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 commented 4 years ago

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

DurandA commented 4 years ago

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

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

jordens commented 4 years ago

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

whitequark commented 4 years ago

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 commented 4 years ago

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

jordens commented 4 years ago

a != 0