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

Optional with clause for invoke #712

Merged
merged 10 commits into from
Oct 7, 2021
Merged

Optional with clause for invoke #712

merged 10 commits into from
Oct 7, 2021

Conversation

rachitnigam
Copy link
Contributor

@rachitnigam rachitnigam commented Oct 6, 2021

Optional with clause for invoke operator. The semantics will require that the assignments within the with clause are active for the duration of invoke operator.

For example

cells {
  mem_a = std_mem_d1(..);
  mem_b = std_mem_d1(..);
}
wires {
  comb group read_mems {
    mem_a.addr0 = 4'd1;
    mem_b.addr0 = 4'd0;
  }
}
control {
  invoke comp(a = mem_a.read_data, b = mem_b.read_data)() with read_mems;
}

The program will ensure that while comp is being executed, the indices from the combinational group read_mems are used to select the memory location.
Before this PR, the only way to implement this functionality is to use a group that first reads the locations in the memory into registers and then to use the values within those registers.

Similarly, this will allow us to generate combinational chains for expressions like this:

(1 + 2) * (3 + 4)

When generating this code, frontends like Dahlia have to use std_mult_pipe structurally because the values from the two adds need to be visible to the inputs of the multiplier:

group do_mul {
  add0.left = 1; add0.right = 2;
  add1.left = 3; add1.right = 4;
  mult.left = add0.out; mult.right = add1.out;
  mult.go = !mult.done;
  do_mul[done] = mult.done;
}

Instead, we'll be able to write:

comb group do_adds {
  add0.left = 1; add0.right = 2;
  add1.left = 3; add1.right = 4;
}
invoke mult(left = add0.out, right = add1.out)() with do_adds;

TODO

  • Add tests
  • Handle invoke-with in remove-comb-groups.
  • Open issue about how compile-invoke can compile invoke-with clauses better but can't do this right now because the optimizations don't work with comb group definitions.

@rachitnigam
Copy link
Contributor Author

@EclecticGriffin I added a correctness test for invoke-with and tried to make interpret_invoke work correctly with invoke-with but seems like it generates the wrong answer. To repro:

fud e --to interpreter-out -s verilog.data tests/correctness/invoke-with.futil.data tests/correctness/invoke-with.futil

Expected output is 21.

The two possibilities for why this doesn't work are:

  1. The implementation of std_mult_pipe doesn't follow the invokeable interface where it's output should latch the value after execution (see Make std_mult variants invokable' #704)
  2. My fix for invoke-with is wrong.

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Absolutely fantastic; this seems like an important advance in making it easier for frontends to generate code for lots of common cases.

@EclecticGriffin
Copy link
Collaborator

Is it the case that is this is replacing the output list on Invoke statements?

@EclecticGriffin
Copy link
Collaborator

Regardless, seems pretty cool! I'll take a look at the interpreter stuff after the meeting and get back to you on what's going wrong

@rachitnigam
Copy link
Contributor Author

rachitnigam commented Oct 7, 2021

Is it the case that is this is replacing the output list on Invoke statements?

No. It just defines assignments that should be active while the component is invoked. Does it subsume output ports?

Edit: Just noticed that the examples above were missing output arguments. Fixed.

@rachitnigam
Copy link
Contributor Author

@EclecticGriffin if the changes to the interpreter look good, I'd like to land this PR.

Copy link
Collaborator

@EclecticGriffin EclecticGriffin left a comment

Choose a reason for hiding this comment

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

Interpreter changes look fine for the moment. There should also probably be an assert in the InvokeInterpreter struct, but I wouldn't worry about it right now. I can deal with that later as needed

@rachitnigam rachitnigam merged commit 7cdebc4 into master Oct 7, 2021
@rachitnigam rachitnigam deleted the invoke-with branch October 7, 2021 16:23
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.

Allow invoke to use with statements to specify combinational connections
3 participants