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

Jump to bottom

nMigen thinks -5 / 2 = 3 #621

Closed
BracketMaster opened this issue Jul 14, 2021 · 7 comments
Closed

nMigen thinks -5 / 2 = 3 #621

BracketMaster opened this issue Jul 14, 2021 · 7 comments
Labels
backend:rtlil bug
Milestone
0.3

Comments

@BracketMaster
Copy link

nMigen thinks -5 / 2 = 3 , but verilator disagrees.
I suspect this is because -5 // 2 evaluates to -3 in Python.

Commands

$python3 div.py
quotient = -3
$verilator -Wno-lint --cc --exe --build sim_main.cpp div.v
$obj_dir/Vdiv
quotient = -2

FIles

div.py

from nmigen import Signal, Module
from nmigen.back import verilog
from nmigen.hdl.ast import signed

## make a module
dividend = Signal(signed(8))
divisor  = Signal(8)
quotient = Signal(signed(8))
counter  = Signal(8)

m = Module()
m.d.comb += quotient.eq(dividend // divisor)
m.d.sync += counter.eq(counter + 1)

## write module to verilog
top = m
with open(f"{__file__[:-3]}.v", "w") as f:
    f.write(verilog.convert(top, ports=[dividend, divisor, quotient, counter]))

# simulate module in python
from nmigen.sim import Simulator
sim = Simulator(m)

def process():
    yield dividend.eq(-5)
    yield divisor.eq(2)
    for tick in range(2):
        yield
    
    print(f"quotient = {(yield quotient)}")

sim.add_clock(1e-6)
sim.add_sync_process(process)
sim.run()

sim_main.cpp

#include "Vdiv.h"
#include "verilated.h"
#include <iostream>
#include <iomanip>
#include <chrono>
#include <cstdint>

  // inspired heavily by code from:
  // https://veripool.org/guide/latest/example_cc.html#example-c-execution
  int main(int argc, char** argv, char** env) {
      VerilatedContext* contextp = new VerilatedContext;
      contextp->commandArgs(argc, argv);
      Vdiv* top = new Vdiv{contextp};

      top->dividend = 5;
      top->divisor = -2;

      top->clk = 0;
      for (int i = 0; i < 20; i++) {
	      top->clk = ~(top->clk);
	      top->eval();
      }

      printf("quotient = %hhd\n", top->quotient);

      delete top;
      delete contextp;
      return 0;
  }
@alanvgreen
Copy link
Contributor

It took me a minute or two to work out what is going on here: the same nMigen code evaluates differently in the nmigen simulator than it does in Verilator.

@whitequark
Copy link
Member

Yes. It's an issue with the emitted Verilog code, I believe; so not specific to Verilator.

@BracketMaster
Copy link
Author

Obviously up to you @whitequark what is canonical nMigen behavior.
But I'd argue that in this case, concerning negative integer division,
verilog's behavior seems a little more reasonable to me:

module arithmetic_operators();

initial begin
  $display (" -5  /  2 = %d", -5/2);
  #10 $finish;
end

endmodule

-5 / 2 = -2

@BracketMaster
Copy link
Author

negative integer division behavior in Python strikes me as less common

@whitequark
Copy link
Member

Obviously up to you @whitequark what is canonical nMigen behavior.

Consider the three snippets: Const(-5 / 2), Const(-5) / 2, and Const(-5) / Const(2). Do you think it is acceptable for any of them to evaluate to different values?

@BracketMaster
Copy link
Author

OK you got me. Probably not worth fighting Python itself about this.

@whitequark whitequark added this to the 0.3 milestone Dec 11, 2021
@whitequark
Copy link
Member

Thanks for the report! This is now fixed by using the upstream Yosys support for flooring division and modulo operators, added in YosysHQ/yosys#1885. As a result, Yosys >=0.10 is now required.

@whitequark whitequark mentioned this issue Feb 3, 2023
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 bug
Milestone
0.3
Development

No branches or pull requests
3 participants
@whitequark @alanvgreen @BracketMaster