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

Add when.cond for getting the current when condition #1694

Merged
merged 2 commits into from
Jan 19, 2021
Merged

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Dec 8, 2020

This is useful for libraries to guard operations implemented via
annotations or BlackBoxes by the current when predicate

@albert-magyar, remember how we discussed this like 3 years ago?

One question I have is how far we should backport this. It's a feature addition so we could just do 3.4.x, but it is a critical feature for verification libraries so it might be nice to go back to 3.2.x.

The main complexity in the implementation is that elsewhen and otherwise are actually parallel with previous conditions that must not be true (rather than being nested as they are in FIRRTL). I originally had altConds as a single Option[() => Bool], but the test caught the issue with elsewhen. Basically, altConds is a List to keep track of the condition of the starting when and prior elsewhens in the same chain so they can be inverted. Nesting is handled inside of when.cond when it accesses the whenStack.

I also used foldRight throughout the implementation so that conditions will flow in the order of the whens rather than being reversed.

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 adds when.cond for determining the current when condition

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Squash

Release Notes

Add when.cond for determining the current when condition

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

This is useful for libraries to guard operations implemented via
annotations or BlackBoxes by the current when predicate
Copy link
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

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

Looks fine. A bit more documentation on alt might be helpful, especially motivation (I assume it's so you can lazily evaluate the inverse, because you expect it will rarely need to happen?)

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

I test this today, works great!
This is really useful for Verification and plugin dev.

@jackkoenig
Copy link
Contributor Author

Because this API is very much a "power user library writer" thing (I actually have struggled to provide useful-looking examples using it other than directly driving a blackbox or annotating the signal), @albert-magyar suggested we put it in a different package like chisel3.advanced or something. Any thoughts/opinions on that @ducky64 @azidar @sequencer?

@ducky64
Copy link
Contributor

ducky64 commented Dec 9, 2020

Something similar did cross my mind: it's a bit of a reflection style API, which we've typically put (maybe a bit haphazardly) in DataMirror. I don't know if that's the right answer, though.

@aswaterman
Copy link
Member

I think putting features that are too important to exclude, but are dangerous or not useful for typical users, into something like chisel3.advanced is a sensible proposal.

Comment on lines +48 to 55
def cond: Bool = {
implicit val compileOptions = ExplicitCompileOptions.Strict
implicit val sourceInfo = UnlocatableSourceInfo
val whens = Builder.whenStack
whens.foldRight(true.B) {
case (ctx, acc) => acc && ctx.localCond()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we get a source locator from the when conditions themselves?

@jackkoenig
Copy link
Contributor Author

We talked about this in dev yesterday, I'm swayed the most by @chick's argument that moving things from experimental has been painful so maybe things like this should be handled more with documentation than anything. Obviously experimental isn't a perfect comparison since chisel3.advanced is somewhere we'd probably not plan to move things out of, but if we did want to then it would be the same problem.

@seldridge asked if there's any prior art for "advanced" packages, and I don't think I know of any. Do other projects have packages for "advanced features" or do they just document things?

@seldridge seldridge requested a review from azidar January 18, 2021 18:56
@jackkoenig jackkoenig added this to the 3.4.x milestone Jan 19, 2021
@jackkoenig jackkoenig merged commit cdb7bb2 into master Jan 19, 2021
@jackkoenig jackkoenig deleted the when.cond branch January 19, 2021 23:55
mergify bot pushed a commit that referenced this pull request Jan 19, 2021
This is useful for libraries to guard operations implemented via
annotations or BlackBoxes by the current when predicate

(cherry picked from commit cdb7bb2)
@mergify mergify bot added the Backported This PR has been backported label Jan 19, 2021
mergify bot added a commit that referenced this pull request Jan 20, 2021
This is useful for libraries to guard operations implemented via
annotations or BlackBoxes by the current when predicate

(cherry picked from commit cdb7bb2)

Co-authored-by: Jack Koenig <[email protected]>
yqszxx pushed a commit to yqszxx/chisel3 that referenced this pull request Jan 25, 2021
)

This is useful for libraries to guard operations implemented via
annotations or BlackBoxes by the current when predicate
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
This Serializer which is implemented
external to the IR node definition
uses a StringBuilder to achieve about a
1.7x performance improvement when serializing.

Eventually, all implementations of the
`serialize` methd should be replaced with
a call to `Serializer.serialize`.

However, for this PR we keep the old
code in place in order to allow for easy
regression testing with the benchmark JAR
like this:
> java -cp utils/bin/firrtl-benchmark.jar \
  firrtl.benchmark.hot.SerializationBenchmark \
  ~/benchmarks/medium.pb 2 5 test

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants