I think the bug is that pack pattern's (excluding the special case of carry chains) should only appear on "internal" nets inside a device. It should not be generated for the connections to the top level ports?

That is one thing which can probably be fixed by adding a port attribute that would allow/disallow annotating the "outward" connection with a pack pattern.

However, the second issue is that you assign these attributes not to wires but to nets. A net can span multiple cells making it impossible to explicitly specify a pack pattern on individual cell-to-cell connections. Here is an example:

module BLOCK(
  input  wire [3:0] lut_in,
  output wire       lut_out,
  output wire       ff1_out,
  output wire       ff2_out
);

  wire w;
  wire w1;
  wire w2;

  // LUT site
  LUT lut (.I(lut_in), .O(w));

  assign w1 = w;
  assign w2 = w;

  // FF sites
  FF ff1 (.I(w1), .O(ff1_out));
  FF ff2 (.I(w2), .O(ff2_out));

  // Combinational LUT output
  assign lut_out = w;

endmodule

Its a LUT that drives 2 FFs. The three wires w, w1 and w2 form a single net that gets the (* PACK *) annotation. When you specify different (* PACK *) attributes on w1 and w2 you get a conflict.

We either need to come up with another way of specifying pack patterns or generate them automatically on all cell-to-cell connections as you have suggested in #75 (comment). Although it is still unclear for me whether the latter approach would be always feasible.

It seems like the net should be decoded into (src, dst) pairs and then auto-detect if a pack pattern is needed for each pair? A pack pattern is needed if neither src nor dst is a top level port?

@mithro There is one exception for that which is a carry-chain pack-pattern that actually has to include top-level port.

I'd like the auto-detection of pack-patterns to be more controlled hence I suggest that we do that only for nets that have the PACK attribute (with no value) provided. Pack-pattern names would be auto-generated basing on cell names and would be specific for each (src, dst) pair.

By default pack-pattern annotation would not include top-level ports unless a port has the PACK attribute on it as well. Since carry-chain pack-patterns must have the same name in all affected pb_types the PACK attribute on a port would take a value of that name.

There is one issue with the proposed cell-to-cell pack-pattern generation: It won't allow to propagate the pack pattern through different levels of pb_type hierarchy. The propagation would require parsing included XMLs as V2X includes XMLs for child cells.

@mkurc-ant -- v2x already separates out the carry-chain pack-patterns from the other pack-patterns. For a carry-chain you use the (* carry *) attribute rather than the (* pack *) attribute.

@mithro I've just realized that the problem is more complicated than that. Restricting (* pack *) to only cell-to-cell connections would disallow specifying pack-patterns that spans multiple levels of pb_type hierarchy. In these cases top-level ports should actually be included.

Another thing is that automatic pack-pattern generation for (src, dst) pairs of a net based on cell names will be always local to a specific pb_type. This will also prevent us from making pack-patterns that cross pb_type hierarchy.

An example case when a pack-pattern must include pb_types from different locations in the hierarchy is model of the QuickLogic EOS-S3 LOGIC cell. In this file there are two pack patterns that should be specified on a connection between a flip-flop leaf pb_type and an output of its parent pb_type: https://github.com/QuickLogic-Corp/symbiflow-arch-defs/blob/c6fab2118bec32b2bf6824d1c1afa92fb277d70d/quicklogic/pp3/primitives/logic/q_frag_modes.sim.v#L21
These pack-pattern names are used throughout the whole LOGIC cell hence automatic pack-pattern name generation is not an option for such a case.

GitHub

QuickLogic-Corp/symbiflow-arch-defs

FOSS architecture definitions of FPGA hardware useful for doing PnR device generation. - QuickLogic-Corp/symbiflow-arch-defs

@mithro I have an idea how to solve pack-patterns for nets with multiple sinks but it would require more work: In Yosys you can use the insbuf pass which inserts $_BUF_ cells between connected wires which effectively makes them separate nets. V2X could process such a netlist and determine which individual wires have (* pack *) attributes and to what cells they connect to (the latter is not possible now). That would allow to propagate the pack-pattern only to a relevant branch of a net.

Rather than putting the (* pack *) on the wire, can we put the (* pack *) on the port of the cell?

So Yosys accepts attribute on cell port connection but unfortunately it discards it. This gets parsed but the attribute content isn't stored anywhere:

  // LUT site
  LUT lut (.I(lut_in), (* PACK="lut_to_ff" *) .O(w));

Did you have a patch for adding surelog support for this type of thing?

I used surelog to get parameters from attributes. I have to check how hard would it be to get attributes from port connections.