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

More Circt intrinsic wrappers (IsX, PlusArgsTest, PlusArgsValue) #2958

Merged
merged 23 commits into from
Apr 8, 2023

Conversation

darthscsi
Copy link
Contributor

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • new feature/API

API Impact

This extends the set of circt intrinsics with Chisel wrappers. These have no impact on code not using them.

Backend Code Generation Impact

N/A

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

Add support for Circt intrinsics.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@jackkoenig jackkoenig mentioned this pull request Feb 14, 2023
14 tasks
@jackkoenig
Copy link
Contributor

I've opened a PR that I think will make it much easier to cleanup the 1 issue with this PR: #3009

* $test$plusargs to test for the existance of the string str in the
* simulator command line.
*/
private class PlusArgsTestIntrinsic[T <: Data](gen: T, str: String) extends ExtModule(Map("FORMAT" -> str)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is gen: T , where T is any data, needed here? Should it just be a Bool type? can you show an @example of something where it fills out a useful Data of some arbitrary type here?

* value as indicated by the format string and a flag for whether the value
* was found.
*/
private class PlusArgsValueIntrinsic[T <: Data](gen: T, str: String) extends ExtModule(Map("FORMAT" -> str)) {
Copy link
Contributor

@mwachs5 mwachs5 Apr 4, 2023

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 take any Data here, vs just a UInt (of some particular width...?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could be reading a string or binary representation of a struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example:

// Generated by CIRCT sifive/1/25/0-469-ge8de64f62-dirty
module PlusArgsValueTop(
  input         clock,
                reset,
  output        io_wf,
  output [31:0] io_wv,
  output        io_xf,
  output [15:0] io_xv_a,
                io_xv_b
);

  logic [31:0]                                     _pargs;
  struct packed {logic [15:0] a; logic [15:0] b; } _pargs_0;
  logic _pf, _pf_0;
  initial begin
    _pf = $value$plusargs("FOO=%d", _pargs);
    _pf_0 = $value$plusargs("BAR=%d", _pargs_0);
  end
  assign io_wf = _pf;
  assign io_wv = _pargs;
  assign io_xf = _pf_0;
  assign io_xv_a = _pargs_0.a;
  assign io_xv_b = _pargs_0.b;
endmodule

* b.value
* }}}
*/
def apply[T <: Data](gen: T, str: String) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

so is the intrinsic handling the unpacking into some complicated data type from the %d which is going to just be an integer? Is this useful vs making that happen explicitly in chisel and make this intrinsic just return a UInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intrinsic returns any verilog type which can be parsed with format strings (and is supported from firrtl). The format string can be used in verilog to pick apart complex specification strings for subfields you are interested in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intrinsic is nothing more than a wrapper for verilog-spec functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, I am echoing Schuyler's comment above that I'm unclear why this can return anything other than a UInt at this level. Users can easily cast from UInt to their appropriate data type in a safer way than this happening automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intrinsic isn't doing this, verilog is. What is safer in moving the casting point? It moves unsafe type conversion further into the user code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I understand that this is meaningful in verilog, I am OK with it. We might want to add something to the scaladoc that in terms of order / unpacking it's equivalent to using a UInt and then .asTypeOf(gen).

@darthscsi darthscsi marked this pull request as draft April 4, 2023 18:29
val xv = Output(UInt(32.W))
})
val tmpw = PlusArgsValue(UInt(32.W), "FOO=%d")
val tmpx = PlusArgsValue(io.xv, "BAR=%d")
Copy link
Contributor

Choose a reason for hiding this comment

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

if we are going to support complicated bundle types then please show a test of that here... but I'm not sure the advantage of having it take any Data vs just a UInt (or SInt possibly through some API...?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, for example is parsing a non-integer substring. We can't express this in firrtl since we don't have real types, but in verilog: $value$plusargs("FREQ+%0F",frequency)

you can also control integer encoding in parsing, parse strings, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you show that being used in the test?

how would we cast something with format string "FREQ+%0F" to a gen: DecoupledIO(UInt(32.W)), something the API will happily let us do right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example with a struct: #2958 (comment)

val xv = Output(UInt(32.W))
})
val tmpw = PlusArgsValue(UInt(32.W), "FOO=%d")
val tmpx = PlusArgsValue(io.xv, "BAR=%d")
Copy link
Contributor

@mwachs5 mwachs5 Apr 4, 2023

Choose a reason for hiding this comment

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

and if the format string is user controlled please show some tests/applications of things that are not of the form "SomeSTring=%d"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To what end? The string is passed by chisel verbatim to firrtl. These are not end-to-end verilog generation tests.

Copy link
Contributor

@mwachs5 mwachs5 Apr 5, 2023

Choose a reason for hiding this comment

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

This is a user API we are designing. I am trying to understand the utility of exposing this string to the user, as well as the utility of allowing arbitrary gen data types. If we have no use for it, and users are only ever going to reasonably get UInts back, then let's remove the ambiguity int he API. If we are saying "sure, you can format your plusarg as a float, or signed number" then what do we expect the users to assign those to?

Why isn't the utility something like PlusArgValueUInt, PlusArgValueSInt.

I'm not even sure what we'd do with a PlusArgValueFloat

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we are trying to formalize https://github.com/chipsalliance/rocket-chip/blob/master/src/main/resources/vsrc/plusarg_reader.v, which does expose the format string, but not the output data type (which is hard-coded to a UInt of a user-specified width).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not trying to formalize that. We are trying to provide access to functionality described in SV 2012 section 21.6. That that blackbox will be directly implementable is a (intended) side effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A missing point is it is not the case that the format has to be "STR=FORMAT", it can be any pattern match.

darthscsi and others added 2 commits April 4, 2023 15:01
darthscsi and others added 2 commits April 4, 2023 15:02
Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Megan Wachs <megan@sifive.com>
@darthscsi darthscsi marked this pull request as ready for review April 4, 2023 20:11
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Nice to see this getting filled out!

My only major question is on the use of a type parameter inside the plus arg intrinsic. Otherwise, looks good.

src/main/scala/chisel3/util/circt/PlusArgsTest.scala Outdated Show resolved Hide resolved
Comment on lines 29 to 35
if (gen.isSynthesizable) {
val inst = Module(new PlusArgsTestIntrinsic(chiselTypeOf(gen), str))
inst.found
} else {
val inst = Module(new PlusArgsTestIntrinsic(gen, str))
inst.found
}
Copy link
Member

Choose a reason for hiding this comment

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

Code nit. It would likely be cleaner to do the maybe synthesizable unboxing explicitly to share some code:

Suggested change
if (gen.isSynthesizable) {
val inst = Module(new PlusArgsTestIntrinsic(chiselTypeOf(gen), str))
inst.found
} else {
val inst = Module(new PlusArgsTestIntrinsic(gen, str))
inst.found
}
val _gen = if (gen.isSynthesizable) {
chiselTypeOf(gen)
} else {
gen
}
val inst = Module(new PlusArgsTestIntrinsic(_gen, str))
inst.found

Alternatively, it would probably be better to have this require a Chisel type and not accept either a Chisel or Hardware type. Ditto for other intrinsics.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we've generally leaned towards making it the user's problem to pass the right thing in (type vs hw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed nicer to be able to pass in the kind of thing you are storing the result in rather than having to get it's type yourself. That way you can do a:

Wire w : UInt<4>
w <= PlusArgsTest(w, "FOO")

Copy link
Contributor

@mwachs5 mwachs5 Apr 5, 2023

Choose a reason for hiding this comment

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

i'd say a more common pattern would be val w = PlusArgsTest(UInt(4.W), "Foo")) (making wires is uncommon).

should we wrap a Default value in this API, if the plus args is not set?

Comment on lines 17 to 20
val io = FlatIO(new Bundle {
val found = Output(UInt(1.W))
val result = Output(gen)
})
Copy link
Member

Choose a reason for hiding this comment

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

You can directly use val found = Output(UInt(1.W)) instead of passing this through FlatIO. That really only becomes necessary when dealing with a BlackBox which dynamically checks that there is a val io.

Same nit as above, Bool is likely better for found.

For result, is this appropriate to take a type parameter? It seems like we wouldn't know what to do with this if it was anything other than a UInt? Would this be better expressed as a UInt of fixed width (for now) or parametric width (later)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @seldridge meant val found = IO(Output(Bool()) (edit mine on the Bool, but the point is you do still need the IO, but you're allowed to have as many IO calls as you want.

Copy link
Contributor

@mwachs5 mwachs5 Apr 5, 2023

Choose a reason for hiding this comment

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

and we could still take a gen: UInt (no type paramterization) letting the user controls the width. Alternatively the old plus arg reader just forced you to specify the width and then it made the UInt with that width, no gen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FlatIO was an old artifact of extmodule. Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take that back, this gives me a bundle for scala purposes but flat io in the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I care about exposing verilog functionality via an intrinsic, not about doing exactly what one code bases blackbox does.

Comment on lines 13 to 14
private class SizeOfIntrinsic[T <: Data](gen: T) extends IntrinsicModule("circt.sizeof") {
val i = IO(Input(gen))
Copy link
Member

Choose a reason for hiding this comment

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

Way cleaner than the explicit annotation. 👍

src/test/scala/chiselTests/util/IsXSpec.scala Outdated Show resolved Hide resolved
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

LGTM

This will help immensely with writing assertions without compiler escape hatches.

it should "Should work for types" in {
val fir = ChiselStage.emitCHIRRTL(new PlusArgsValueTop)
println(fir)
(fir.split('\n').map(_.trim) should contain).inOrder(
Copy link
Member

Choose a reason for hiding this comment

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

The PR can do a takeWhile(_ != '@') to strip the source locators before the trim. (Generally it's bad to match on the source locator.)

@darthscsi darthscsi enabled auto-merge (squash) April 8, 2023 18:23
@darthscsi darthscsi merged commit 024cbfc into chipsalliance:main Apr 8, 2023
@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Apr 11, 2023
jared-barocsi pushed a commit that referenced this pull request Apr 11, 2023
This extends the set of circt intrinsics with Chisel wrappers. These have no impact on code not using them.
azidar pushed a commit that referenced this pull request Apr 11, 2023
This extends the set of circt intrinsics with Chisel wrappers. These have no impact on code not using them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants