Removal of conditional code macro definitions for ABC/ABC9

[litghost]

In earlier versions of the ABC/ABC9 support in yosys, there was a define _ABC or _ABC9 that could be used to conditionally select code that is suitable for ABC/ABC9 versus other uses.

-D_ABC9

yosys/techlibs/xilinx/synth_xilinx.cc

Lines 286 to 292 in d4212d1

	std::string read_args;
	if (vpr)
	read_args += " -D_EXPLICIT_CARRY";
	if (abc9)
	read_args += " -D_ABC9";
	read_args += " -lib +/xilinx/cells_sim.v";
	run("read_verilog" + read_args);

or

-D_ABC

yosys/techlibs/xilinx/synth_xilinx.cc

Lines 286 to 289 in 7959e9d

	if (vpr)
	run("read_verilog -lib -D_ABC -D_EXPLICIT_CARRY +/xilinx/cells_sim.v");
	else
	run("read_verilog -lib -D_ABC +/xilinx/cells_sim.v");

It appears these were removed in 1dc2260

The question is, why was this removed? In cases where conditional code is desired and/or needed for different needs (abc/abc9/simulation), what is the recommended way to support this?

[eddiehung]

That macro was something that was necessary to work around some ABC9 modelling limitations. It's removal was because -- to my knowledge -- I had fixed those limitations and can now interpret the unmodified simulation model. Please be more direct about what the breakage is, or propose a new PR showing why they would still be necessary for your use case.

[litghost]

You may not remember, but you are actually the one who suggested using _ABC when this came up last time, specifically for:

`ifdef _ABC
  wire CI0 = CYINIT | CI;
`else
  wire CI0 = (CI === 1'bz) ? CYINIT :
      (CYINIT === 1'bz) ? CI :
      (CI | CYINIT);
`endif

https://github.com/SymbiFlow/yosys/blob/6bccd35a41ab82f52f0688478310899cfec04e08/techlibs/xilinx/cells_sim.v#L279-L285

The underlying hardware in this case is not precisely modeled in either branch, as there is in fact a bitstream configured mux between CIN and CYINIT, rather than a logic |. The last time we talked, you proposed the above code as a reasonable way to express the hardware for either ABC/ABC9 or simulation. Given that _ABC (or _ABC9) is now gone, this naturally no longer works.

The reason for the above logic is in the event that the client of CARRY4 specifies CIN or CYINIT (but not both). This is valid given that there is a static mux between CIN/CYINIT and the carry chain.

[eddiehung]

I honestly had completely forgotten about that. I would be in favour of putting it back in with a comment that explains its necessity -- go ahead and do so in a PR. It does feel a little fishy to detect unconnected ports by checking equality with 1'bz though.

[litghost]

It does feel a little fishy to detect unconnected ports by checking equality with 1'bz though.

I'm not sure verilog provides a superior method. It is worth noting that Vivado's unisim uses a similiar construction: https://github.com/Xilinx/XilinxUnisimLibrary/blob/master/verilog/src/unisims/CARRY4.v#L73-L74 The unisim construction is actually a more robust construction, because it handles the "default" case, where both ports are disconnected.

I'm open to other suggestions for how to model this kind of construction.