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.

Sign up for GitHub

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

Jump to bottom

Instrumentation widget #53

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Instrumentation widget #53

wants to merge 4 commits into from

Conversation

edwardcwang
Copy link
Member

(Don't merge until #50 is merged since this PR depends on it)

@edwardcwang
Copy link
Member Author

Passes midas-top:

$ make run-bmark-tests EMUL=vcs RISCV=$RISCV
cd /scratch/edwardw/midas-top/generated-src/f1/RocketChip2GExtMem/ && \
./MidasTop /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mm.riscv +sample=/scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mm.riscv.sample +max-cycles=100000000 +mm_MEM_LATENCY=10 \
2> /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mm.riscv.out && [ $PIPESTATUS -eq 0 ]
Chronologic VCS simulator copyright 1991-2016
Contains Synopsys proprietary information.
Compiler version L-2016.06-1_Full64; Runtime version L-2016.06-1_Full64;  Nov 20 23:34 2017
[...]
Compiler version L-2016.06-1_Full64; Runtime version L-2016.06-1_Full64;  Nov 20 23:37 2017

matmul(cid, nc, 16, input1_data, input2_data, results_data); barrier(nc): 46618 cycles, 11.3 cycles/iter, 1.5 CPI

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mm.riscv.out 	 after 356158 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/spmv.riscv.out 	 after 137278 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mt-vvadd.riscv.out 	 after 111038 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/median.riscv.out 	 after 24382 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/multiply.riscv.out 	 after 56638 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/qsort.riscv.out 	 after 342334 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/towers.riscv.out 	 after 23230 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/vvadd.riscv.out 	 after 17470 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/dhrystone.riscv.out 	 after 251838 cycles

  [ PASSED ] /scratch/edwardw/midas-top/output/f1/RocketChip2GExtMem/mt-matmul.riscv.out 	 after 68286 cycles

@edwardcwang edwardcwang mentioned this pull request Nov 21, 2017
Open
davidbiancolin
davidbiancolin reviewed Nov 22, 2017
View reviewed changes
Copy link
Contributor

@davidbiancolin davidbiancolin left a comment

Choose a reason for hiding this comment

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

Generally, i like the direction you're heading, and some of the readability improvements you've made.

Let's see how this evolves once you get the boring utils working.

src/main/scala/midas/Config.scala
@@ -28,7 +30,7 @@ class SimConfig extends Config((site, here, up) => {
case KeepSamplesInMem => true
case CtrlNastiKey => NastiParameters(32, 32, 12)
case MemNastiKey => NastiParameters(64, 32, 6)
case EndpointKey => EndpointMap(Seq(new SimNastiMemIO, new SimAXI4MemIO))
case EndpointKey => EndpointMap(Seq(new SimNastiMemIO, new SimAXI4MemIO, new SimInstrumentationIO))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be added to the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, may I inquire as to why this is the case? From what I saw it seems that if there are no InstrumentationIOs, it shouldn't cause any backwards-incompatible changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mainly, because it's not even clear yet that Endpoint-style matching will be the ultimate way you bind your widget to the target.

src/main/scala/midas/widgets/InstrumentationIO.scala
super.genHeader(base, sb)
import CppGenerationUtils._

// Generate all_ready()
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 explain why this needs to be so customized? Also, do you have to emit macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

These make it a lot easier to pull data out of the instrumentation widget and was really useful for the testcase (and perhaps also in the general case) since it helped remove a lot of boilerplate code (e.g. to mark all the widgets as ready) which depended on the exact elements of the bundle.

I tried to make these functions but then 1) something in the build system complains about duplicate definition; 2) it would depend on simif_t and including simif.h here doesn't seem like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok we should talk about this in person.

src/main/scala/midas/core/FPGATop.scala
@@ -93,21 +93,30 @@ class FPGATop(simIoType: SimWrapperIO)(implicit p: Parameters) extends Module wi
b.elements.toList foreach { case (name, wire) =>
loop(target.elements(name), wire)
}
case (target: Record, r: Record) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate your efforts to better support Record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbiancolin davidbiancolin davidbiancolin left review comments

@donggyukim donggyukim Awaiting requested review from donggyukim

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edwardcwang @davidbiancolin