-
Notifications
You must be signed in to change notification settings - Fork 277
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
[Pipeline] Allow explicitly named registers and passthroughs #5602
Conversation
Reflected in HW lowering, and used by e.g. the register materialization pass to carry register names through multiple stages. Still a draft since i want to get peoples opinion on the HW lowering before i change tests. Example lowering: ```mlir hw.module @testNaming(%go: i1, %clk: i1, %rst: i1) -> (out: i1) { %0:2 = pipeline.scheduled "MyPipeline"(%a0 : i1 = %go) clock(%c = %clk) reset(%r = %rst) go(%g = %go) -> (out: i1) { // The register will be named after the namehint on the source operation. %add = comb.add %a0, %a0 : i1 pipeline.stage ^bb1 regs("myNamed" = %add : i1, %add : i1) pass("myNamed" = %add : i1, %add : i1) ^bb1(%reg0 : i1, %reg1 : i1, %pass0 : i1, %pass1 : i1, %s1_valid: i1): pipeline.stage ^bb2 regs(%reg0 : i1, %reg1 : i1) pass(%pass0 : i1, %pass1 : i1) ^bb2(%reg01 : i1, %reg11 : i1, %pass01 : i1, %pass11 : i1, %s2_valid: i1): pipeline.return %reg01 : i1 } hw.output %0#0 : i1 } // Becomes hw.module @testNaming_MyPipeline(%a0: i1, %enable: i1, %clk: i1, %rst: i1) -> (out: i1, valid: i1) { %testNaming_MyPipeline_s0.myNamed, %testNaming_MyPipeline_s0.out1, %testNaming_MyPipeline_s0.myNamed_pass, %testNaming_MyPipeline_s0.pass1, %testNaming_MyPipeline_s0.valid = hw.instance "testNaming_MyPipeline_s0" @testNaming_MyPipeline_s0(a0: %a0: i1, enable: %enable: i1, clk: %clk: i1, rst: %rst: i1) -> (myNamed: i1, out1: i1, myNamed_pass: i1, pass1: i1, valid: i1) %testNaming_MyPipeline_s1.out0, %testNaming_MyPipeline_s1.out1, %testNaming_MyPipeline_s1.pass0, %testNaming_MyPipeline_s1.pass1, %testNaming_MyPipeline_s1.valid = hw.instance "testNaming_MyPipeline_s1" @testNaming_MyPipeline_s1(myNamed: %testNaming_MyPipeline_s0.myNamed: i1, out1: %testNaming_MyPipeline_s0.out1: i1, myNamed_pass: %testNaming_MyPipeline_s0.myNamed_pass: i1, pass1: %testNaming_MyPipeline_s0.pass1: i1, enable: %testNaming_MyPipeline_s0.valid: i1, clk: %clk: i1, rst: %rst: i1) -> (out0: i1, out1: i1, pass0: i1, pass1: i1, valid: i1) hw.output %testNaming_MyPipeline_s1.out0, %testNaming_MyPipeline_s1.out1 : i1, i1 } hw.module @testNaming_MyPipeline_s0(%a0: i1, %enable: i1, %clk: i1, %rst: i1) -> (myNamed: i1, out1: i1, myNamed_pass: i1, pass1: i1, valid: i1) { %0 = comb.add %a0, %a0 : i1 %s0_myNamed_reg = seq.compreg sym @s0_myNamed_reg %0, %clk : i1 %s0_reg1 = seq.compreg sym @s0_reg1 %0, %clk : i1 %false = hw.constant false %s0_valid = seq.compreg sym @s0_valid %enable, %clk, %rst, %false : i1 hw.output %s0_myNamed_reg, %s0_reg1, %0, %0, %s0_valid : i1, i1, i1, i1, i1 } hw.module @testNaming_MyPipeline_s1(%myNamed: i1, %out1: i1, %myNamed_pass: i1, %pass1: i1, %enable: i1, %clk: i1, %rst: i1) -> (out0: i1, out1: i1, pass0: i1, pass1: i1, valid: i1) { %s1_reg0 = seq.compreg sym @s1_reg0 %myNamed, %clk : i1 %s1_reg1 = seq.compreg sym @s1_reg1 %out1, %clk : i1 %false = hw.constant false %s1_valid = seq.compreg sym @s1_valid %enable, %clk, %rst, %false : i1 hw.output %s1_reg0, %s1_reg1, %myNamed_pass, %pass1, %s1_valid : i1, i1, i1, i1, i1 } hw.module @testNaming(%go: i1, %clk: i1, %rst: i1) -> (out: i1) { %testNaming_MyPipeline.out, %testNaming_MyPipeline.valid = hw.instance "testNaming_MyPipeline" @testNaming_MyPipeline(a0: %go: i1, enable: %go: i1, clk: %clk: i1, rst: %rst: i1) -> (out: i1, valid: i1) hw.output %testNaming_MyPipeline.out : i1 } ``` Exports as ```sv module testNaming_MyPipeline( input a0, enable, clk, rst, output out, valid ); wire _testNaming_MyPipeline_s0_myNamed; wire _testNaming_MyPipeline_s0_out1; wire _testNaming_MyPipeline_s0_myNamed_pass; wire _testNaming_MyPipeline_s0_pass1; wire _testNaming_MyPipeline_s0_valid; testNaming_MyPipeline_s0 testNaming_MyPipeline_s0 ( .a0 (a0), .enable (enable), .clk (clk), .rst (rst), .myNamed (_testNaming_MyPipeline_s0_myNamed), .out1 (_testNaming_MyPipeline_s0_out1), .myNamed_pass (_testNaming_MyPipeline_s0_myNamed_pass), .pass1 (_testNaming_MyPipeline_s0_pass1), .valid (_testNaming_MyPipeline_s0_valid) ); testNaming_MyPipeline_s1 testNaming_MyPipeline_s1 ( .myNamed (_testNaming_MyPipeline_s0_myNamed), .out1 (_testNaming_MyPipeline_s0_out1), .myNamed_pass (_testNaming_MyPipeline_s0_myNamed_pass), .pass1 (_testNaming_MyPipeline_s0_pass1), .enable (_testNaming_MyPipeline_s0_valid), .clk (clk), .rst (rst), .out0 (out), .out1_0 (valid), .pass0 (/* unused */), .pass1_0 (/* unused */), .valid (/* unused */) ); endmodule module testNaming_MyPipeline_s0( input a0, enable, clk, rst, output myNamed, out1, myNamed_pass, pass1, valid ); reg s0_myNamed_reg; reg s0_reg1; always_ff @(posedge clk) begin s0_myNamed_reg <= 1'h0; s0_reg1 <= 1'h0; end reg s0_valid; always_ff @(posedge clk) begin if (rst) s0_valid <= 1'h0; else s0_valid <= enable; end assign myNamed = s0_myNamed_reg; assign out1 = s0_reg1; assign myNamed_pass = 1'h0; assign pass1 = 1'h0; assign valid = s0_valid; endmodule module testNaming_MyPipeline_s1( input myNamed, out1, myNamed_pass, pass1, enable, clk, rst, output out0, out1_0, pass0, pass1_0, valid ); reg s1_reg0; reg s1_reg1; always_ff @(posedge clk) begin s1_reg0 <= myNamed; s1_reg1 <= out1; end reg s1_valid; always_ff @(posedge clk) begin if (rst) s1_valid <= 1'h0; else s1_valid <= enable; end assign out0 = s1_reg0; assign out1_0 = s1_reg1; assign pass0 = myNamed_pass; assign pass1_0 = pass1; assign valid = s1_valid; endmodule module testNaming( input go, clk, rst, output out ); testNaming_MyPipeline testNaming_MyPipeline ( .a0 (go), .enable (go), .clk (clk), .rst (rst), .out (out), .valid (/* unused */) ); endmodule ```
My personal preference is to omit "_reg" suffixes, because some synthesis tools add "_reg" suffixes during synthesis, and it becomes awkward to have registered named "x_reg_reg". |
in terms of naming, I have no preferences. Where's the |
and why is "myNamed" in both the regs and pass? just to test that names dont conflict? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % name changes
|
||
// Returns the register name for a given register index. | ||
StringAttr getRegisterName(unsigned regIdx) { | ||
if(auto names = getOperation()->getAttrOfType<ArrayAttr>("registerNames")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant you access this through the ods generated accessor method?
} else { | ||
// For the return op, we just inherit the names of the top-level pipeline | ||
// as stage output names. | ||
llvm::copy(pipeline.getOutputNames().getAsRange<StringAttr>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you assert that this is only return ops?
if (names) { | ||
if (auto nameAttr = names[idx].dyn_cast<StringAttr>(); | ||
nameAttr && !nameAttr.strref().empty()) | ||
p << nameAttr << " = "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to emit the name if it's empty?
This probably isn't the best example, seeing as it gets canonicalized away... |
Reflected in HW lowering, and used by e.g. the register materialization pass to carry register names through multiple stages.
Still a draft since i want to get peoples opinion on the HW lowering before i change tests, e.g. things like whether there should be a
_reg
suffix on stage module outputs which are driven by registers (and likewise e.g._reg_in
for stage inputs that arrive from a prior stage that was a registered output).Example lowering:
Exports as