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

Documentation updates #2056

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

Documentation updates #2056

wants to merge 18 commits into from

Conversation

SwRaw
Copy link
Contributor

@SwRaw SwRaw commented Nov 26, 2024

resolves #___

Summary:

*What is being changed and why?

Outcomes:

What is the result of this change? What components of the project does it affect?

Notable changes:

Are there any changes that are of particular importance?

Testing and Environment:

What environment are you targeting (OS, ROCm version, Python versions, etc.)?

*What testing did you do to ensure this change will integrate successfully?

@@ -145,28 +155,30 @@ Line **[D]** shows a child catalog for gfx906, similar to the gfx900 catalog. Ho
type: Hardware # [_A]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bstefanuk Should I remove underscore from _A, _B ..etc.? The reference given below has no underscore.

Copy link
Contributor

Choose a reason for hiding this comment

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

I originally intended for these to identify open and close positions for a range of lines, but I'm not attached to this. Feel free to remove and update to your liking.

@SwRaw
Copy link
Contributor Author

SwRaw commented Dec 9, 2024

@bstefanuk Please review

@@ -25,7 +25,8 @@ subtrees:
- file: src/api-reference/embedded-data
- file: src/api-reference/tensile-create-library-api
- file: src/api-reference/utilities
- file: src/reference/environment-variables
- file: src/reference/environment-variables
title: Environment variables

Choose a reason for hiding this comment

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

Reference section doesn't align:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -5,7 +5,7 @@
.. _installation:

********************************************************************
Installation
Tensile installation
********************************************************************

Install ROCm

Choose a reason for hiding this comment

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

image

Choose a reason for hiding this comment

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

remove "only" above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced it with:
Alternatively, export the path exclusively for your current shell session, using export PATH=/opt/rocm/bin/:$PATH.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, the following wording is clear,

Alternatively, export the path for your current shell session using `export PATH=/opt/rocm/bin/:$PATH`

Developers will understand that the "current shell session" won't persist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -5,7 +5,7 @@
.. _installation:

********************************************************************
Installation
Tensile installation
********************************************************************

Install ROCm

Choose a reason for hiding this comment

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

There are two headings in a row. Either remove this heading.
Perhaps better to put an introduction on this page that explains what the purpose is.

Choose a reason for hiding this comment

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

Review for "we" in this document

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two headings in a row. Either remove this heading. Perhaps better to put an introduction on this page that explains what the purpose is.

There is only one heading. The first one you see in red is a removed line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review for "we" in this document

I found "we" in this statement:

"For demonstration purposes, we use the sample tuning file available in Tensile/Configs/rocblas_sgemm_example.yaml"
Even though I somehow feel that this sounds appropriate, I have replaced it with the following (if we need to avoid "we" at all costs):

"For demonstration purposes, the sample tuning file available in Tensile/Configs/rocblas_sgemm_example.yaml is used"

PS: I had read in google style guide, that we can use "we" in such cases. Let me know if I should refrain from "we".

Copy link
Contributor

Choose a reason for hiding this comment

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

No opinion on this.

@@ -8,17 +8,19 @@
Nomenclature

Choose a reason for hiding this comment

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

Did you consider other terms?
Perhaps Glossary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glossary sounds good. Updated.


.. math::
C = \alpha A B + \beta C
where :math:`\alpha` and :math:`\beta` are scalars and :math:`A` and :math:`B` are optionally transposed input matrices.
In the preceding equation, :math:`\alpha` and :math:`\beta` are scalars and :math:`A` and :math:`B` are optionally transposed input matrices.

Choose a reason for hiding this comment

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

Suggested change
In the preceding equation, :math:`\alpha` and :math:`\beta` are scalars and :math:`A` and :math:`B` are optionally transposed input matrices.
In the preceding equation, :math:`\alpha` and :math:`\beta` are scalars, and :math:`A` and :math:`B` are optionally transposed input matrices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 74 to 75
adds up to four input dimensions and contracts them along one dimension. This cancels out two dimensions, leading to a 2-dimensional result.
When an index shows up in multiple tensors, those tensors must be the same size along with the dimension, however, they can have different strides.

Choose a reason for hiding this comment

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

Suggested change
adds up to four input dimensions and contracts them along one dimension. This cancels out two dimensions, leading to a 2-dimensional result.
When an index shows up in multiple tensors, those tensors must be the same size along with the dimension, however, they can have different strides.
adds up to four input dimensions, and contracts them along one dimension. This cancels out two dimensions, leading to a 2-dimensional result.
When an index shows up in multiple tensors, those tensors must be the same size along with the dimension. However, they can have different strides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

When an index shows up in multiple tensors, those tensors must be the same size along with the dimension, however, they can have different strides.

There are three categories of indices or dimensions used in the problems supported by Tensile: free, batch and bound.
**Tensile only supports problems with at least one pair of free indices.**
There are three categories of indices or dimensions used in the problems supported by Tensile: free, batch, and bound.

Choose a reason for hiding this comment

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

Suggested change
There are three categories of indices or dimensions used in the problems supported by Tensile: free, batch, and bound.
Three categories of indices or dimensions are used in the problems supported by Tensile: free, batch, and bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +11 to +16
To avoid the overhead associated with loading code object files including initialization time and the memory footprint of the loaded code object files,
Tensile provides a mechanism to load only a subset of the code object files produced during a build, at runtime.
To achieve this, it must be determined which code object file to load.
To determine the preferred kernel and the code object file containing the selected kernel,
the ``TensileHost`` library utilizes a process named `Solution selection`.
This process uses a hierarchical structure to efficiently search for kernels based on hardware, problem size, and transpose, among others.

Choose a reason for hiding this comment

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

Suggested change
To avoid the overhead associated with loading code object files including initialization time and the memory footprint of the loaded code object files,
Tensile provides a mechanism to load only a subset of the code object files produced during a build, at runtime.
To achieve this, it must be determined which code object file to load.
To determine the preferred kernel and the code object file containing the selected kernel,
the ``TensileHost`` library utilizes a process named `Solution selection`.
This process uses a hierarchical structure to efficiently search for kernels based on hardware, problem size, and transpose, among others.
To avoid the overhead associated with loading code object files, including initialization time and the memory footprint of the loaded code object files,
Tensile provides a mechanism to load only a subset of the code object files produced during a build at runtime.
To achieve this, it must be determined which code object file to load.
To determine the preferred kernel and the code object file containing the selected kernel,
the ``TensileHost`` library utilizes `Solution selection` process.
This process uses a hierarchical structure to efficiently search for kernels based on hardware, problem size, and transpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The search for kernel is based not only on hardware, problem size and transpose ,hence I think we should mention "among others".

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the former of the two options here, and agree with @SwRaw that, since there are other parameters that are involved, we keep "among others".

Catalog hierarchy
=================
Solution selection catalog hierarchy
=====================================

.. figure:: ../../assets/msl.svg
:alt: Master Solution Library hierarchy
:align: center

Solution selection catalog heirarchy for gfx900 and gfx90a

Choose a reason for hiding this comment

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

Suggested change
Solution selection catalog heirarchy for gfx900 and gfx90a
Solution selection catalog hierarchy for gfx900 and gfx90a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The solution selection catalog is general irrespective of architecture. As an example, gfx900 and 90a are used in the image. Hence, I don't think its a great idea to add the architectures to the heading.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for the figure caption, and since this figure is demonstrating the hierarchy for these two architectures, it seems appropriate to keep "gfx900" and "gfx90a" in the caption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we already have "gfx900" and gfx90a in the image caption. I too prefer not to mention it in the heading.

Copy link
Contributor

@bstefanuk bstefanuk left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I think some reversions and minor fixes are in order, but nothing major.
I still don't support prefixing our headings with "Tensile", I really don't see what it's adding.

Thank you for your work!

Comment on lines 7 to 18
**************************************
Tensile's solution selection catalogs
**************************************

To avoid the overhead associated with loading code object files including initialization time and the memory footprint of the loaded code object files,
Tensile provides a mechanism to load only a subset of the code object files produced during a build, at runtime.
To achieve this, it must be determined which code object file to load.
To determine the preferred kernel and the code object file containing the selected kernel,
the ``TensileHost`` library utilizes a process named `Solution selection`.
This process uses a hierarchical structure to efficiently search for kernels based on hardware, problem size, and transpose, among others.

For efficient lookup at runtime, the kernel metadata must be organized in a hierarchical schema in a serialized file named `solution selection catalog` [1]_.
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the original drafting of this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only improved the original draft in terms of readability, adding context and writing style. Please let me keep the newer version.

docs/src/conceptual/solution-selection-catalogs.rst Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@
.. _installation:

********************************************************************
Installation
Tensile installation
********************************************************************

Install ROCm
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, the following wording is clear,

Alternatively, export the path for your current shell session using `export PATH=/opt/rocm/bin/:$PATH`

Developers will understand that the "current shell session" won't persist.

Comment on lines +11 to +16
To avoid the overhead associated with loading code object files including initialization time and the memory footprint of the loaded code object files,
Tensile provides a mechanism to load only a subset of the code object files produced during a build, at runtime.
To achieve this, it must be determined which code object file to load.
To determine the preferred kernel and the code object file containing the selected kernel,
the ``TensileHost`` library utilizes a process named `Solution selection`.
This process uses a hierarchical structure to efficiently search for kernels based on hardware, problem size, and transpose, among others.
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the former of the two options here, and agree with @SwRaw that, since there are other parameters that are involved, we keep "among others".

Catalog hierarchy
=================
Solution selection catalog hierarchy
=====================================

.. figure:: ../../assets/msl.svg
:alt: Master Solution Library hierarchy
:align: center

Solution selection catalog heirarchy for gfx900 and gfx90a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for the figure caption, and since this figure is demonstrating the hierarchy for these two architectures, it seems appropriate to keep "gfx900" and "gfx90a" in the caption.

@@ -5,7 +5,7 @@
.. _installation:

********************************************************************
Installation
Tensile installation
********************************************************************

Install ROCm
Copy link
Contributor

Choose a reason for hiding this comment

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

No opinion on this.

@@ -5,7 +5,7 @@
.. _installation:

********************************************************************
Installation
Tensile installation
Copy link
Contributor

Choose a reason for hiding this comment

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

We are still wanting to redundantly add "Tensile" in front of all our headings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpaoletti Please confirm.


.. math::
C = \alpha A B + \beta C
where :math:`\alpha` and :math:`\beta` are scalars and :math:`A` and :math:`B` are optionally transposed input matrices.
In the preceding equation, :math:`\alpha` and :math:`\beta` are scalars and, :math:`A` and :math:`B` are optionally transposed input matrices.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert to using "where"

docs/src/install/installation.rst Outdated Show resolved Hide resolved
Comment on lines 5 to 11
.. _glossary:

************
Nomenclature
************
*********
Glossary
*********

This topic lists and describes the frequently used terms in the Tensile documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't actually a glossary. To me, a glossary is an alphabetical list of terms, which this isn't. I think this should be reverted to "Nomenclature" as it's a document that helps clarify the system of naming used within the project.

There are some terms that could be added to a glossary section at the end of this file, that's fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpaoletti Should I revert to "Nomenclature"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants