rdaly525 / coreir

BSD 3-Clause "New" or "Revised" License
101 stars 24 forks source link

Avoid u wires #948

Closed leonardt closed 4 years ago

leonardt commented 4 years ago

In some cases, we were still generating the _$_U wires introduced during the inlinePassThrough logic even though we were marking with metadata that they should be inlined, here's one instance:

module MuxWithDefaultWrapper_9_32_8_0 (
    input [31:0] I [8:0],
    input [7:0] S,
    input [0:0] EN,
    output [31:0] O
);
reg [31:0] MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out;
wire [3:0] _$_U592;
always @(*) begin
if (_$_U592 == 0) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[0];
end else if (_$_U592 == 1) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[1];
end else if (_$_U592 == 2) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[2];
end else if (_$_U592 == 3) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[3];
end else if (_$_U592 == 4) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[4];
end else if (_$_U592 == 5) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[5];
end else if (_$_U592 == 6) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[6];
end else if (_$_U592 == 7) begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[7];
end else begin
    MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out = I[8];
end
end

assign _$_U592 = S[3:0];
assign O = (S < 8'h09) & EN[0] ? MuxWrapper_9_32_inst0$Mux9x32_inst0$coreir_commonlib_mux9x32_inst0_out : 32'h00000000;
endmodule

The issue is that the recent change the introduced blocking of inlining for instance inputs (so we didn't get expressions inside the instance statements) was marking instance input identifiers as "do not inline". This was overriding the metadata forcing the inlining of these mantle wires introduced by inlinePassThrough.

This change improves the logic to ensure that the inline wire metadata overrides this default "do not inline" behavior. However, this isn't quite perfect yet since it causes a concat expression to be inlined into a module instance statement (see the change in tests/gtest/golds/slice_combview.v). Fortunately, this doesn't present a semantic problem since the tools we've been encountering don't seem to mind this syntax, so I propose for now we add this change (in order to reduce the amount of code noise in the generated output, this is immediately annoying for debugging for our current users, while the prevention of inlining into instance inputs isn't problematic for anyone yet), and do a followup change to improve the inlining logic to avoid this case where a mantle wire is inlined into an instance input with a concat expression (in this case, I think we should introduce a temporary "_" wire instead of the generic unique mantle wire name that is confusing).

Also, in the process of this change, I changed the wires set to be per-module, rather than global, which is actually how it should be (since two modules with the same wire names could clobber each other).

leonardt commented 4 years ago

I think one general solution for handling the above case is to do a pass after inlining to lift any instance inputs that are not simply an identifier/index/slice node into an assignment to a wire.

rsetaluri commented 4 years ago

I think one general solution for handling the above case is to do a pass after inlining to lift any instance inputs that are not simply an identifier/index/slice node into an assignment to a wire.

I like this.

leonardt commented 4 years ago

It turns out as a general pass this is trickier than I expected. Basically, to do this lifting, we either need to infer the type of the driver (could be ac complicated expression) or use the type of the input port (need to do global analysis of all the modules in the scope). I think there's a much simpler way to solve this in the coreir code generation phase (where the type information is already present), so I'm going to try that just for code simplicity.

leonardt commented 4 years ago

(That doesn't negate the value of this pass as generally useful if the analysis can be done, but in terms of priority, I think the simpler solution is best since there's more important stuff to work on and I don't foresee this being a major issue)