Place Design error when using if/else statements in verilog

Question

I'm trying to do one of the exercises on the book I'm studying from. The idea is to turn on/off LEDS depending on switches. But I got a place design error when tried to run implementation (synthesis ok):

ERROR:[Place 30-574] Poor placement for routing between an IO pin and BUFG. If this sub optimal condition is acceptable for this design, you may use the CLOCK_DEDICATED_ROUTE constraint in the .xdc file to demote this message to a WARNING. However, the use of this override is highly discouraged. These examples can be used directly in the .xdc file to override this clock rule.

I'm learning FPGA design so clearly not an expert at it. After trying to "debug" and isolate the error, I found that this works:

module led_pattern(led, l_sw, r_sw);
output reg [15:0] led;
input [1:0] r_sw;
input [15:14] l_sw;
always @ (r_sw or l_sw)
begin
if (r_sw[0] == 1'b1)
 begin
 led[0] = 1'b1;
 led[1] = 1'b1;
 end
else
 begin
 led[0] = 1'b0;
 led[1] = 1'b0;
 end
if (r_sw[0] == 1'b0 && r_sw[1] == 1'b0 && l_sw[14] == 1'b0 && l_sw[15] == 1'b0)
 begin
 led[7] = 1'b1;
 led[8] = 1'b1;
 end
else
 begin
 led[7] = 1'b0;
 led[8] = 1'b0;
 end
end
endmodule

but this doesn't (it triggers the above quoted error):

module led_pattern(led, l_sw, r_sw);
output reg [15:0] led;
input [1:0] r_sw;
input [15:14] l_sw;
always @ (r_sw or l_sw)
begin
if (r_sw[0] == 1'b1)
 begin
 led[0] = 1'b1;
 led[1] = 1'b1;
 end
else if (r_sw[0] == 1'b0)
 begin
 led[0] = 1'b0;
 led[1] = 1'b0;
 end
if (r_sw[0] == 1'b0 && r_sw[1] == 1'b0 && l_sw[14] == 1'b0 && l_sw[15] == 1'b0)
 begin
 led[7] = 1'b1;
 led[8] = 1'b1;
 end
else
 begin
 led[7] = 1'b0;
 led[8] = 1'b0;
 end
end
endmodule

Note that the only difference is in the former I used else statement and in the latter I used "else if". Finally, the part of top module where I'm instantiating led_pattern is:

module top(LED, AN, CA, CB, CC, CD, CE, CF, CG, SW);
// Ports declaration
input [15:0] SW;
output [7:0] AN;
output CA,CB,CC,CD,CE,CF, CG;
output [15:0] LED;
wire [6:0] seg = {CG, CF, CE, CD, CC, CB, CA}; 
reg [3:0] seg_value;
led_pattern sys(LED, SW[15:14], SW[1:0]);

didn't put the rest of the code because might not be necessary.

Note: I'm not using clocks here (at least not on purpose, maybe yes by accident?). led is used for LEDS on NEXYS4 DDR and l_sw, r_sw are switches from the board. This is purely combinatorial.

What could be causing that place error?

• CapnJJ: Did you create a constraint file (.xdc) and map the IO pins accordingly? Do you have a clock in this design? The module you show is combinatorial (assuming l_sw or r_sw is not a clock)
• Miguel Duran Diaz: I'll edit it to clarify. But yes, I do have a constraint file (I've done other designs involving switches and leds). When I say it doesn't work I mean it triggers the quoted error. And nope, not using clocks at all (actually I have not learned that yet).
• CapnJJ: It is strange to me that the RTL difference does that, because, that error is associated with placement constraints. It seems that PAR is trying to infer/resolve a clock on an input pin that has no route to BUFG. If you have no clock, or need for BUFG otherwise, just use the CLOCK_DEDICATED_ROUTE to demote it to a warning as stated, or reassign that input to a pin that has a BUFG. To better understand why, I would compare the two synthesis files and see what it did different to that else if that PAR thinks there is a clock in the latter design
• The Photon: I don't see any else in the first example. Did you accidentally paste the same code twice instead of two different examples? It would probably help to add comments showing us exactly where to look for the differences between your two examples.
• CapnJJ: @ThePhoton first if-else versus if-else if for r_sw[0]

Answer 1

The tool is trying to turn some of your signals into clocks (as indicated by its attempt to infer a BUFG), based on your always blocks. If you're not doing sequential logic (e.g., with clocks), you shouldn't be using such blocks at all. Instead, write what you want directly:

assign led[0] = r_sw[0];
assign led[1] = r_sw[0];
assign led[7] = !r_sw[0] & !r_sw[1] & !l_sw[14] & !l_sw[15];
assign led[8] = !r_sw[0] & !r_sw[1] & !l_sw[14] & !l_sw[15];

etc.

In general, whenever you find yourself writing if (x) then y <= 1 else Y <= 0;, you should replace it with y <= x;. I see this all the time, and it drives me up a wall!

• Miguel Duran Diaz: Thanks! I did that way because the exercise on the book said "use if/else statements" so I did. In that case, is there any other way to use if/else avoiding the use of always? or is there a "correct" way of using if/else in combinatorial designs?
• The Photon: @MiguelDuranDiaz, your code will probably work fine in a simulator. The error from Xilinx has to do with being able to achieve high speed and reduce resource consumption in the actual hardware. At the level you're currently learning, you can probably ignore this error.

Answer 2

I agree with Dave's answer. The preferred solution is to not use procedural blocks to infer combinatorial logic...except in certain well-defined cases.

The reason is that the synthesis tool is actually quite dumb. It doesn't really understand what your code is doing. It just looks for patterns in your code that it's been programmed to understand, and transforms them to a netlist to be processed further by the mapping and place & route programs.

So your synthesis tool has probably been programmed to recognize a multiplexer coded as a procedural block

always @(S) begin
 case(S)
 case 0: a <= b0;
 case 1: a <= b1;
 endcase
end

but it has probably not been programmed to recognize what you tried to produce: a simple wire connection or buffer coded as a procedural block.

If in doubt, you should read the Synthesis Guide for your tool to see the patterns that are accepted by that tool, and what logic they actually infer.