Skip to content
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

[CombToAIG] Add shl/shru/shrs lowering #8067

Merged
merged 4 commits into from
Jan 14, 2025
Merged

[CombToAIG] Add shl/shru/shrs lowering #8067

merged 4 commits into from
Jan 14, 2025

Conversation

uenoku
Copy link
Member

@uenoku uenoku commented Jan 11, 2025

This commit implements lowering for shift operations. They are currently lowered into mux-trees with a gate condition for out-of-bounds, e.g.:

hw.module @shift3(in %lhs: i3, in %rhs: i3, out out_shl: i3, out out_shr: i3, out out_shrs: i3) {
  %0 = comb.shl %lhs, %rhs : i3
  %1 = comb.shru %lhs, %rhs : i3
  %2 = comb.shrs %lhs, %rhs : i3
  hw.output %0, %1, %2 : i3, i3, i3
}
$ circt-opt --pass-pipeline="builtin.module(hw.module(convert-comb-to-aig{additional-legal-ops=comb.xor,comb.or,comb.and,comb.mux,comb.add,comb.icmp},convert-aig-to-comb),export-verilog)" bar.mlir

module shift3(  // bar.mlir:2:1
  input  [2:0] lhs,     // bar.mlir:2:22
               rhs,     // bar.mlir:2:35
  output [2:0] out_shl, // bar.mlir:2:49
               out_shr, // bar.mlir:2:66
               out_shrs // bar.mlir:2:83
);

  wire [2:0] _GEN = {3{lhs[2]}};        // bar.mlir:5:8
  assign out_shl =
    rhs < 3'h3
      ? (rhs[1] ? (rhs[0] ? 3'h0 : {lhs[0], 2'h0}) : rhs[0] ? {lhs[1:0], 1'h0} : lhs)
      : 3'h0;   // bar.mlir:2:35, :3:8, :6:3
  assign out_shr =
    rhs < 3'h3
      ? (rhs[1] ? (rhs[0] ? 3'h0 : {2'h0, lhs[2]}) : rhs[0] ? {1'h0, lhs[2:1]} : lhs)
      : 3'h0;   // bar.mlir:2:35, :4:8, :6:3
  assign out_shrs =
    rhs < 3'h3
      ? (rhs[1]
           ? (rhs[0] ? _GEN : {3{lhs[2]}})
           : rhs[0] ? {{2{lhs[2]}}, lhs[1]} : {lhs[2], lhs[1:0]})
      : _GEN;   // bar.mlir:2:35, :5:8, :6:3
endmodule

This commit implements lowering for shift operations.
They are currently lowered into mux-trees.
lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
test/Conversion/CombToAIG/comb-to-aig-arith.mlir Outdated Show resolved Hide resolved
test/Conversion/CombToAIG/comb-to-aig-arith.mlir Outdated Show resolved Hide resolved
lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved

return nodes;
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of if-else checks for left/right signed/unsigned shifts that contain most of the code of this pattern. Wouldn't it make sense to break it up into one pattern per shift op to make it more readable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I refactored into different structs and used callbacks.

lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
@uenoku uenoku requested a review from maerhart January 13, 2025 21:28
@uenoku uenoku force-pushed the dev/hidetou/shift branch from 8c71278 to 67853ad Compare January 13, 2025 21:45
@uenoku
Copy link
Member Author

uenoku commented Jan 13, 2025

@maerhart Thank you for the great feedback! I totally agree it's nicer to just separate lowering patterns. I refactor the lowering pattern to use callbacks to get padding and extraction for each shifted amount.

Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot easier to read. Thanks!

lib/Conversion/CombToAIG/CombToAIG.cpp Outdated Show resolved Hide resolved
@uenoku uenoku merged commit 75473ab into main Jan 14, 2025
4 checks passed
@uenoku uenoku deleted the dev/hidetou/shift branch January 14, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants