-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vioProbe uses its clock lazily #2551
Conversation
With this patch, for: module Test where
import Clash.Explicit.Prelude
import Clash.Cores.Xilinx.VIO
import Clash.Xilinx.ClockGen
topEntity :: Clock System -> Reset System -> Vec 4 (Signal System Bit) -> Vec 4 (Signal System Bit)
topEntity clkIn rst inp = out
where
(clk,_) = clockWizard @System @System (SSymbol @"clk_wiz_0") clkIn rst
out = map (vioProbe ("vio_in":>Nil) ("vio_out":>Nil) 0 clk) inp we get the following Verilog: /* AUTOMATICALLY GENERATED VERILOG-2001 SOURCE CODE.
** GENERATED BY CLASH 1.7.0. DO NOT MODIFY.
*/
`default_nettype none
`timescale 100fs/100fs
module topEntity
( // Inputs
input wire clkIn // clock
, input wire rst // reset
, input wire inp_0
, input wire inp_1
, input wire inp_2
, input wire inp_3
// Outputs
, output wire result_0
, output wire result_1
, output wire result_2
, output wire result_3
);
wire [1:0] c$case_scrut;
wire [3:0] inp;
wire [3:0] result;
assign inp = {inp_0, inp_1, inp_2,
inp_3};
// clockWizard begin
clk_wiz_0 clockWizard_inst
(.clk_in1 (clkIn)
,.reset ( rst)
,.clk_out1 (c$case_scrut[1])
,.locked (c$case_scrut[0]));
// clockWizard end
// map begin
genvar i;
generate
for (i=0; i < 4; i = i + 1) begin : map
wire map_in;
assign map_in = inp[i*1+:1];
wire map_out;
wire c$arg0;
assign c$arg0 = c$case_scrut[1:1];
topEntity3 topEntity3_0
( .result (map_out)
, .c$arg (c$arg0)
, .c$arg_0 (map_in) );
assign result[i*1+:1] = map_out;
end
endgenerate
// map end
assign result_0 = result[3:3];
assign result_1 = result[2:2];
assign result_2 = result[1:1];
assign result_3 = result[0:0];
endmodule i.e. there is only one |
Can we capture this in a helper function? Maybe something like: vioProbe# (markUsed -> !_inputNames) (markUsed -> !_outputNames) (markused -> !_initialOutputProbeValues) !(markUsed -> _clk) = ... My worry is that it is pretty hard to apply this workaround consistently. We should have a simple, consistent way we can apply everywhere to every argument. |
You can use
|
Okay makes sense. In that case: does an alias (e.g., |
We use the bang-pattern/seq to mark the argument as "used", the use of I think we should just add this use of |
Fixes #2532
So #2532 shows up when we have a primitive that doesn't actually use its clock argument and we use a bang-pattern on an otherwise unused argument. If we look at the demand signature of
vioProbe#
before the patch we see:where
Because the
_clk
argument is both definitely evaluated and used once so there is no need to share. This in turn makes GHC want to inline the_clk
argument in applications ofvioProbe#
.After the patch we get the following demand signature:
Now the
clk
argument is marked as evaluated lazily, and can possibly be shared. This in turn makes GHC not want to inline theclk
argument in applications ofvioProbe#
.If we decide to merge this PR we should do two things: