Cautionary Tale -- Chisel/ISE issue

[shunshou]

So I've spent the last n days debugging someone else's Chisel/Verilog, which generally exhibited this symptom:

Verilog testbench passes in Vivado (behavioral + post synthesis) Behavioral Verilog testbench passes in ISE 12.x-14.2 Post synthesis, Verilog testbench fails -- whole modules are optimized out during synthesis with lots of "TXXX is removed b/c ____" warning messages (painful to debug; ISE is required for Virtex 5 support, which is what the NI SDR platform uses)

I eventually found the cause. See the following (valid) Verilog (simplified version of what Chisel emits) that fails post synthesis:

IO: input [4:0] io_simplecomparein, wire [4:0] T722; assign io_simplecompare = $signed(io_simplecomparein) < $signed(T722); assign T722 = {2'h3, 3'h5};

---

Something that should be functionally equivalent (and actually passes post-synthesis simulation with ISE):

IO: input [4:0] io_simplecomparein wire [4:0] T722; assign T722 = {2'h3, 3'h5}; assign io_simplecompare = $signed(io_simplecomparein) < $signed(T722);

---

Basically, the signed comparison was wrong when T722 was assigned below the < operation, even though it should be perfectly valid to assign T722 below the < operation. This seems to only be a problem when T722 is a constant.

---

Edit: Ok, so the check is on me. I wasn't sure how signed comparisons worked, so I always sign extended comparator inputs to match widths... (other person was using my extended types) and unfortunately, doing so creates an intermediate node aka T722, that gets assigned to a constant after it is used in the comparison.

Your typical SInt comparison just applies $signed(constant) within the < operation, so you'd never run into this problem...

Either way, the sign extension should be valid, but ISE runs into problems because of the particular ordering of emitted Verilog.

[albert-magyar]

If it's that simple that's a really embarrassing bug in ISE.

[shunshou]

I think it was probably something that wasn't caught until the Vivado upgrade b/c no one actually writes code like that... despite it being perfectly valid... >> <<

1) I guess people who know what's going on with $signed wouldn't try to play it safe and manually sign extend (my bad; here's when being conservative about tool capabilities backfired...) -- this is still somewhat questionably problematic if I had two Fixed with mismatched fractional width (where one was also a lit) 2) If they did want to sign extend a constant, they'd just represent it with the right # of bits to begin with, rather than doing something like SInt(-3), which would give the representation with the least # of bits. 3) If they started with a constant with minimal # of bits and then chose to sign extend, they would probably do it in a more logical way i.e. assign T722 before the comparison...

[aswaterman]

Sounds like it would benefit everyone if you reported this to Xilinx.