Move Vivado Mitigations to be ifdef-guarded
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
We currently have two Vivado-centric mitigations to address pretty egregious bugs in Vivado tooling. One of these is mitigated with a Vivado-specific SystemVerilog attribute.
Change the SystemVerilog attribute to be ifdef guarded with a macro that is only defined for Vivado. (I don't know what this is, but it's probably XILINX.)
E.g., consider the following FIRRTL circuit:
FIRRTL version 4.2.0
circuit Foo:
public module Foo:
input clock: Clock
input r: {addr: UInt<3>, en: UInt<1>, flip data: UInt<8>}
input w: {addr: UInt<3>, en: UInt<1>, data: UInt<8>, mask: UInt<1>}
mem memory:
data-type => UInt<8>
depth => 16
reader => r
writer => w
read-latency => 0
write-latency => 1
read-under-write => undefined
connect memory.r.addr, r.addr
connect memory.r.en, r.en
connect memory.r.clk, clock
connect r.data, memory.r.data
connect memory.w.addr, w.addr
connect memory.w.en, w.en
connect memory.w.clk, clock
connect memory.w.data, w.data
connect memory.w.mask, w.mask
Change this so that, when compiled it produces:
// Generated by CIRCT firtool-1.108.0-23-g231dea049
// VCS coverage exclude_file
module memory_16x8(
input [3:0] R0_addr,
input R0_en,
R0_clk,
output [7:0] R0_data,
input [3:0] W0_addr,
input W0_en,
W0_clk,
input [7:0] W0_data
);
`ifdef XILINX
(* ram_style = "distributed" *)
`endif
reg [7:0] Memory[0:15];
reg _R0_en_d0;
reg [3:0] _R0_addr_d0;
always @(posedge R0_clk) begin
_R0_en_d0 <= R0_en;
_R0_addr_d0 <= R0_addr;
end // always @(posedge)
always @(posedge W0_clk) begin
if (W0_en & 1'h1)
Memory[W0_addr] <= W0_data;
end // always @(posedge)
assign R0_data = _R0_en_d0 ? Memory[_R0_addr_d0] : 8'bx;
endmodule
module Foo(
input clock,
input [2:0] r_addr,
input r_en,
output [7:0] r_data,
input [2:0] w_addr,
input w_en,
input [7:0] w_data,
input w_mask
);
memory_16x8 memory_ext (
.R0_addr ({1'h0, r_addr}),
.R0_en (r_en),
.R0_clk (clock),
.R0_data (r_data),
.W0_addr ({1'h0, w_addr}),
.W0_en (w_en & w_mask),
.W0_clk (clock),
.W0_data (w_data)
);
endmodule
As part of this, investigate if there are Vivado-specific macros that it defines, e.g., if XILNIX is actually defined. If no such define exists, then we may need to define one.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by locating the CIRCT code that emits the Vivado-specific SystemVerilog attribute and inspect how generated Verilog is tested. Investigate which macro Vivado defines, including whether XILINX is correct; if none exists, determine where a macro should be defined. Done means the attribute is wrapped in the requested conditional guard and the generated output matches the example when appropriate.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100