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.

Sign up for GitHub

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

Jump to bottom

(~True) instead of (not True) can break a design #159

Closed
whitequark opened this issue Jul 24, 2019 · 1 comment
Closed

(~True) instead of (not True) can break a design #159

whitequark opened this issue Jul 24, 2019 · 1 comment
Labels
backend:RTLIL improvement
Milestone
0.1

Comments

@whitequark
Copy link
Contributor

whitequark commented Jul 24, 2019 •
edited

I just discovered this bug in some oMigen code:

--- bad/software/glasgow/gateware/i2c.py
+++ good/software/glasgow/gateware/i2c.py
@@ -142,7 +142,7 @@ class I2CMaster(Module):
                 If(stb,
                     NextValue(pin_o, 1)
                 ).Elif(pin_o == 1,
-                    If(~stretch | (pin_i == 1),
+                    If((not stretch) | (pin_i == 1),
                         NextState(next_state),
                         *exprs
                     )

It translates to this Verilog:

--- good.v      2019-07-24 20:20:50.179566232 +0000
+++ bad.v       2019-07-24 20:21:05.291656753 +0000
@@ -1190,7 +1190,7 @@
                                i2cmastersubtarget_sda_o_i2cmaster_next_value_ce4 <= 1'd1;
                        end else begin
                                if ((i2cmastersubtarget_sda_o == 1'd1)) begin
-                                       if ((1'd1 | (i2cmastersubtarget_sda_i == 1'd1))) begin
+                                       if ((1'sd1 | $signed({1'd0, (i2cmastersubtarget_sda_i == 1'd1)}))) begin
                                                i2cmaster_next_state <= 2'd3;
                                        end
                                end

Given that (n)Migen signals can be negated only with ~ and booleans can be negated only with not, this seems to be a potential source of serious bugs. Unfortunately I'm not sure what can be done to fix it (since by the time the value is coerced to an nMigen constant, its boolean-ness is long lost), and nMigen is indeed still susceptible to this issue:

>>> C(~True)
(const 2'sd-2)
>>> C(~True) | (1==0)
(| (const 2'sd-2) (const 1'd0))
>>> (C(~True) | (1==0)).shape()
(2, True)
whitequark added a commit to GlasgowEmbedded/glasgow that referenced this issue Jul 24, 2019
@whitequark
gateware.i2c: make clock stretching actually work.
2dd8afe 
Oops... see m-labs/nmigen#159.
@whitequark
Copy link
Contributor Author

Resolution: using a negative value as an operand in m.If should generate a warning.

@whitequark whitequark added this to the 0.1 milestone Aug 3, 2019
@whitequark whitequark changed the title (~False) instead of (not False) can break a design (~True) instead of (not True) can break a design Aug 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:RTLIL improvement
Projects
None yet
Milestone
0.1
Development

No branches or pull requests
1 participant
@whitequark