llvm / llvm/circt

Move Vivado Mitigations to be ifdef-guarded

Open
#8,310 0 comments 0 reactions 0 assignees View on GitHub

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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.