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

23 simulation traces are not understandable for human being #43

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

Conversation

yuqisun
Copy link
Collaborator

@yuqisun yuqisun commented Dec 16, 2024

Attched log sample of VectorCGRAKingMeshRTL_test, please help review, thanks. (There may some trace logs be missed, too many, will update during developing, WDYT)

VectorCGRAKingMeshRTL_test_log.txt

Also sync changes from master.

yuqisun and others added 24 commits December 9, 2024 22:58
…erstandable-for-human-being' into 23-simulation-traces-are-not-understandable-for-human-being
@yuqisun yuqisun requested a review from tancheng December 19, 2024 14:47
@tancheng
Copy link
Owner

Hi @yuqisun, thanks for the PR,

+--------------+----------------+---------+------+-----+
|port_direction|     payload    |predicate|bypass|delay|
+--------------+----------------+---------+------+-----+
|     NORTH    |0000000000000000|    0    |   0  |  0  |
+--------------+----------------+---------+------+-----+
  • Can we distinguish the port_direction into inport_direction and outport_direction?
outport: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
predicate_in: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
  • Can we have the meaningful content inside those array? Say, outport: [N, S, -, -, ..., -]?
===>
- channel_recv:

+----------------+---------+------+-----+
|     payload    |predicate|bypass|delay|
+----------------+---------+------+-----+
|0000000000000000|    0    |   0  |  0  |
+----------------+---------+------+-----+
|0000000000000000|    0    |   0  |  0  |
+----------------+---------+------+-----+
  • What is above channel_recv, is this necessary to be shown? If yes, can we provide its name?

@yuqisun
Copy link
Collaborator Author

yuqisun commented Dec 20, 2024

Hi,

channel_recv is from:
[ x.recv.msg.__dict__ for x in s.channel ] <=== s.channel = [ ChannelRTL( DataType ) for _ in range( num_xbar_outports ) ] <=== num_xbar_outports = num_fu_inports + num_connect_outports.

So channel should be the lines of crossbar, channel_recv should be the inports which in green in below picture, am I correct?

image

And there's also channel_send: [ x.send.msg.__dict__ for x in s.channel ] which should be the outports in blue in picture?

@tancheng
Copy link
Owner

Hi,

channel_recv is from: [ x.recv.msg.__dict__ for x in s.channel ] <=== s.channel = [ ChannelRTL( DataType ) for _ in range( num_xbar_outports ) ] <=== num_xbar_outports = num_fu_inports + num_connect_outports.

So channel should be the lines of crossbar, channel_recv should be the inports which in green in below picture, am I correct?

image

And there's also channel_send: [ x.send.msg.__dict__ for x in s.channel ] which should be the outports in blue in picture?

Nope. Channel actually includes registers. The channel.recv is the blue one, the channel.send is the one after the yellow register. Imagine the channel as a pipe, it doesn't contain the xbar, it connects to the xbar.

@yuqisun
Copy link
Collaborator Author

yuqisun commented Dec 25, 2024

How to use line_trace and verbose_trace in *_test.py

There're 2 kinds of formats of trace log:

  1. line_trace: refers to a single-line trace
  2. verbose_trace: prints more details, such as ctrl_mem ...

For example:
cgra/translate/VectorCGRAKingMeshRTL_test.py

def line_trace( s ):
    # verbose trace (test without verilog), uncomment below to use verbose_trace
    # verbosity = 1
    # return s.dut.verbose_trace(verbosity = verbosity)
    return s.dut.line_trace()

Note: --test-verilog mode requires line_trace always as it uses line_trace in python file which is generated by PyMTL instead of defined in VectorCGRA.

Copy link
Owner

@tancheng tancheng left a comment

Choose a reason for hiding this comment

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

.github/workflows/python-package.yml Outdated Show resolved Hide resolved
cgra/CGRAKingMeshRTL.py Outdated Show resolved Hide resolved
@tancheng tancheng added documentation Improvements or additions to documentation new feature New feature or request labels Dec 25, 2024
@tancheng tancheng linked an issue Dec 25, 2024 that may be closed by this pull request
mem/ctrl/CtrlMemRTL.py Outdated Show resolved Hide resolved
tile/TileRTL.py Outdated Show resolved Hide resolved
tile/TileRTL.py Outdated Show resolved Hide resolved
tile/TileRTL_constant.py Outdated Show resolved Hide resolved
tile/TileSeparateCrossbarRTL.py Outdated Show resolved Hide resolved
tile/TileSeparateCrossbarRTL.py Outdated Show resolved Hide resolved
cgra/CGRAKingMeshRTL.py Outdated Show resolved Hide resolved
@yuqisun yuqisun force-pushed the 23-simulation-traces-are-not-understandable-for-human-being branch from 79d15c1 to 9ed1abb Compare December 26, 2024 01:51
@yuqisun
Copy link
Collaborator Author

yuqisun commented Dec 26, 2024

Thanks!

CgraMemRightAndBottomRTL good.
RingMultiCgraRingCtrlMemRTL pending on CtrlMemDynamicRTL and DataMemWithCrossbarRTL, will add tmr.

lib/util/common.py Outdated Show resolved Hide resolved
mem/ctrl/CtrlMemRTL.py Outdated Show resolved Hide resolved
tile/TileRTL.py Outdated Show resolved Hide resolved
2.

recv_ctrl_msg_list = [ recv_ctrl_sub_header, recv_ctrl_msg_dict ] -> recv_ctrl_msg_list = [recv_ctrl_sub_header, recv_ctrl_msg_dict]
3. put line_trace() before verbose()
mem/data/DataMemRTL.py Outdated Show resolved Hide resolved
# str = "||".join([ x.element.line_trace() for x in s.tile ])
# str += " :: [" + s.data_mem.line_trace() + "]"
res = "||\n".join([(("[tile" + str(i) + "]: ") + x.line_trace() + x.ctrl_mem.line_trace())
for (i,x) in enumerate(s.tile)])
res += "\n :: SouthMem [" + s.data_mem_south.line_trace() + "] \n"
res += "\n :: EastMem [" + s.data_mem_east.line_trace() + "] \n"
return res


def verbose_trace( s, verbosity = 1 ):
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the unnecessary space, (keep the one before and after =).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

systolic/CgraMemRightAndBottomRTL.py Outdated Show resolved Hide resolved
@@ -81,6 +81,7 @@ def done(s):
# return True

def line_trace(s):
# return s.dut.verbose_trace(verbosity = 2)
Copy link
Owner

Choose a reason for hiding this comment

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

If this is the way to invoke the verbose_trace(), let's enable it by default for this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

verbose_trace works for -xvs --tb=short but not --test-verilog, it looks up line_trace(), can test this with -xvs --tb=short for RingMultiCgraRingCtrlMemRTL_test.py, current is pytest --tb=short -sv ../scale_out/translate/RingMultiCgraRingCtrlMemRTL_test.py --test-verilog --dump-vtb --dump-vcd?

Copy link
Owner

Choose a reason for hiding this comment

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

  • Then the xxx_test.py within translate/ folder can have line_trace(), while the one in test/ can have vervose_trace() enabled by default.

  • Or I guess actually you can control it based on the flag?

  def line_trace(s, verbosity=0):
    if verbosity = 0:
      return s.dut.line_trace(verbosity)
    if verbosity = 1:
      return s.dut.verbose_trace(verbosity)

......

dut = Top()

if flags not contains --test-verilog:
  dut.set_param('top.x.y.z.line_trace', verbosity=1) # Or use a global variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, you both are right, controlling by set_param is better, I should use it from the beginning.

Added a line in *_test.py th.set_param('top.dut.line_trace', verbosity = 1), will enable verbose_trace by default and when there's --test-verilog, will ignore verbose itself.

tile/test/TileSeparateCrossbarRTL_test.py Outdated Show resolved Hide resolved
@@ -80,6 +80,7 @@ def check_parity(s):
return True

def line_trace(s):
# return s.dut.verbose_trace(verbosity = 2)
Copy link
Owner

Choose a reason for hiding this comment

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

Enable by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There're both -xvs --tb=short and --tb=short --test-verilog --dump-vtb --dump-vcd in testcases, if enable verbose_trace by default, can I remove and ignore --tb=short --test-verilog --dump-vtb --dump-vcd case in action workflow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make it by set_param.

@tancheng
Copy link
Owner

Hi @yuqisun, I am currently working on a huge cleanup. Plz let me merge first... So then you can sync and merge. Sorry about the inconvenience.

You can move to #44, thanks~!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[P2] Simulation traces are not understandable for human being
3 participants