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

request to improve the generality of STF LIB #3

Open
jeffnye-gh opened this issue May 28, 2024 · 10 comments
Open

request to improve the generality of STF LIB #3

jeffnye-gh opened this issue May 28, 2024 · 10 comments

Comments

@jeffnye-gh
Copy link

I'm promoting a case for more generality in STF LIB records. I believe more generality improves likelihood of broader adoption of the lib.

The STF specification makes assumptions about record formats etc, which precludes using an unmodified STF_LIB, and associated tools outside a range of ISAs. Addition of USER defined settings would help.

ATM the library is strongly organized around the assumptions of RISC-V. There are enums for the others but attempting to implement tracing for the unsupported variants of known ISAs, implement tracing for user defined ISAs, or a custom extensions, can not be done without forking the library.

I believe in some cases these restrictions could be relaxed, with little impact. In other cases these changes would be visible, but I believe these can be handled programmatically through modification to header information.

STF_INST_MEM_ACCESS: as I understand recent comments, this will be documented to require a byte based sizing. bits is more general, particularly in accelerators, not directly related but this is in the same spirit as STF_BUS_MASTER_ACCESS with understands the concept of user created ACCEL (accelerators I presume). Changing to bits allows more generality in particular accelerators with bit addressable memory.

STF_VLEN_CONFIG: VLEN is a RISC-V only naming convention, there are other vector implementations outside RISC-V
STF_PROTOCOL_ID would benefit from a USER enum
STF_ISA would benefit from a USER enum, or much wider support for other ISAs. There is a long list.
STF_INST_REG: limits scalar results to 64 bits, also bakes in VLEN which is a RISC-V only naming convention
STF_INST_MICROOP: this assumes fracture, 1:N where N>= 1, request that a new record type be created for the fusion case, or modify this record type to not assume 1:N
STF_INST_32/16: this bakes in support for only two instruction sizes. Request a more general approach, or some mechanism that supports a variety of encoding sizes.

ProtocolIdRecord would benefit from an explicit USER protocol. We will not always be using TileLink.
@bdutro
Copy link
Contributor

bdutro commented May 28, 2024

STF_INST_MEM_ACCESS: as I understand recent comments, this will be documented to require a byte based sizing. bits is more general, particularly in accelerators, not directly related but this is in the same spirit as STF_BUS_MASTER_ACCESS with understands the concept of user created ACCEL (accelerators I presume). Changing to bits allows more generality in particular accelerators with bit addressable memory.

I'm ok with generalizing on bits here, though I think in that case we should probably increase the width of the size field. I think we could remove the attributes field (unless someone has come up with a good use-case for it) and extend the size field to 32 bits. I'm also thinking it might make sense to combine the MEM_ACCESS and MEM_CONTENT records together into a single record - MEM_CONTENT can't appear without a MEM_ACCESS record anyway, and it makes handling zero-length memory accesses like prefetches cleaner.

STF_VLEN_CONFIG: VLEN is a RISC-V only naming convention, there are other vector implementations outside RISC-V

That's fine, we can rename it to STF_VECTOR_REGISTER_WIDTH or something similarly generic.

STF_PROTOCOL_ID would benefit from a USER enum

Sounds good.

STF_ISA would benefit from a USER enum, or much wider support for other ISAs. There is a long list.

Also sounds fine, and if anyone wants to extend the list they're more than welcome.

STF_INST_REG: limits scalar results to 64 bits, also bakes in VLEN which is a RISC-V only naming convention

I don't really want to rely on >64 bit datatypes for portability reasons, so I think the best way forward here would be to add new fields for register element size (in bits) and # of elements (1 for scalar, >1 for vector). We would also then be able to remove STF_VLEN_CONFIG entirely.

STF_INST_MICROOP: this assumes fracture, 1:N where N>= 1, request that a new record type be created for the fusion case, or modify this record type to not assume 1:N

I'm leaning towards adding a fusion-specific record.

STF_INST_32/16: this bakes in support for only two instruction sizes. Request a more general approach, or some mechanism that supports a variety of encoding sizes.

Opcodes are the most common record type, so making these records variable-length would have a noticeable impact on trace file sizes. On the other hand, making bespoke record types for every possible instruction length wouldn't be feasible. I'm leaning towards collapsing everything into one variable-length STF_INST_OPCODE record type, but we'll need to do some work to make sure it doesn't balloon file sizes too much.

All in all, these changes are all significant enough that we'd probably have to treat this as a v2.0 release. We've also been kicking around some ideas about adding timing information to instruction traces, which would also likely involve breaking backwards compatibility but also make it possible to unify protocol traces and instruction traces.

@jeffnye-gh
Copy link
Author

Sounds good to me.

Let me know if we/condor can help with back filling for coding tests, coding for stf_tools, docs, etc.

If it makes sense to you we'd be happy to contribute where we can.

@bdutro
Copy link
Contributor

bdutro commented May 28, 2024

Let me know if we/condor can help with back filling for coding tests, coding for stf_tools, docs, etc.

If it makes sense to you we'd be happy to contribute where we can.

That would be awesome. If you want to get started on something while I scope out this effort, STF has badly needed tests for a long time. We should start with the library first, and having tests in place for the current version will help ensure that 2.0 is healthy.

@jeffnye-gh
Copy link
Author

jeffnye-gh commented May 29, 2024 via email

@bdutro
Copy link
Contributor

bdutro commented May 29, 2024

Correct me if I'm wrong, I believe that there isn't a ctest based
regression environment for stf_lib at the moment. If this is true I
propose we/condor construct that for stf_lib as the initial development. We
are doing the same for dromajo, maybe we can share some code. I realize
this is likely not a lot of code but we can share what we map for dromajo.

You are correct, and that would be great!

Once the common infrastructure is done then I think we would focus on the
STF features we use the most.

That would be perfect.

@jeffnye-gh
Copy link
Author

jeffnye-gh commented Jun 12, 2024 via email

@bdutro
Copy link
Contributor

bdutro commented Jun 12, 2024

Yes, that sounds like a good plan.

@jeffnye-gh
Copy link
Author

Stan has posted a PR for to the stf_lib repo. He tells me he does not have an option to assign a reviewer.

Can you take a look?

@bdutro
Copy link
Contributor

bdutro commented Jun 14, 2024

I added myself and Knute as reviewers

@jeffnye-gh
Copy link
Author

jeffnye-gh commented Nov 4, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bdutro @jeffnye-gh