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

[MooreToCore] Support string constants #7628

Closed
fabianschuiki opened this issue Sep 25, 2024 · 6 comments
Closed

[MooreToCore] Support string constants #7628

fabianschuiki opened this issue Sep 25, 2024 · 6 comments
Assignees
Labels
good first issue Good for newcomers Moore

Comments

@fabianschuiki
Copy link
Contributor

Add a conversion for the moore.string_constant op to MooreToCore: https://chipsalliance.github.io/sv-tests-results/?v=circt_verilog+11.10+string_bit_array

@jpinot
Copy link
Contributor

jpinot commented Oct 19, 2024

Hey @fabianschuiki, I'd like to give it a shot! Could you assign me?

@fabianschuiki
Copy link
Contributor Author

Sure thing! Thanks a lot for taking a stab at this 😃. Let me know if you run into any trouble.

@jpinot
Copy link
Contributor

jpinot commented Oct 27, 2024

Hi @fabianschuiki! Sorry for the delayed response—it took me a bit to dive into the CIRCT fundamentals to get oriented 😺. To tackle this, I referenced the Moore test results and tried converting moore::StringConstantOp to hw::ConstantOp by translating StringRef into a bit vector, also I tryed using hw::ArrayCreateOp. Here’s the resulting LLHD:

$ ./bin/circt-verilog string_bit_array.sv
module {
  hw.module @top() {
    %0 = llhd.constant_time <0ns, 0d, 1e>
    %c0_i80 = hw.constant 0 : i80
    %c1415934836_i32 = hw.constant 1415934836 : i32
    %c0_i112 = hw.constant 0 : i112
    %a = llhd.sig %c0_i112 : i112
    llhd.process {
      %1 = comb.concat %c0_i80, %c1415934836_i32 : i80, i32
      llhd.drv %a, %1 after %0 : !hw.inout<i112>
      llhd.halt
    }
    hw.output
  }
}

It seems that this module should work as expected since it represents a constant string, which shouldn’t be modified. Still, I’d really appreciate your thoughts on (a) what the expected correct output should look like to confirm I’m on track, and (b) whether there might be a cleaner way to handle the StringRef to bit vector conversion. So far, I’ve only managed to iterate through the string and concatenate the bytes, but I’d love to know if there’s a more elegant approach.

Thanks so much for your help guidance!

@fabianschuiki
Copy link
Contributor Author

Hey @jpinot, this looks really nice! I think your approach of converting the characters in the string into a large hw.constant is great!

jpinot added a commit to jpinot/circt that referenced this issue Oct 30, 2024
Lower string constant ops to constant op in the hw dialect.
jpinot added a commit to jpinot/circt that referenced this issue Nov 4, 2024
Lower string constant ops to constant op in the hw dialect.
jpinot added a commit to jpinot/circt that referenced this issue Nov 5, 2024
Lower string constant ops to constant op in the hw dialect.
jpinot added a commit that referenced this issue Nov 6, 2024
Lower string constant ops to constant op in the hw dialect.
@jpinot
Copy link
Contributor

jpinot commented Nov 6, 2024

Hey @fabianschuiki, once again thanks for your support 🥳 . Should this issue be close?

@fabianschuiki
Copy link
Contributor Author

Yeah sounds great! Thanks for landing that PR 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers Moore
Projects
None yet
Development

No branches or pull requests

2 participants