-
Notifications
You must be signed in to change notification settings - Fork 12
Amend coherent-pointer proposal as per new guidelines of design requested #29
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
Amend coherent-pointer proposal as per new guidelines of design requested #29
Conversation
csyonghe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
Are we good to just merge the proposal changes or should we wait for other team-members to review? |
|
|
||
| * C/C++ – `const int* ptr` or `int const*` both mean the underlying data pointed to is constant | ||
| * This will be equivlent to `Access::Read` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking change:
add one sub-bulletin for int* const ptr, equivalent to const Ptr<> ptr.
also typo: equivlent => equivalent
| { | ||
| ReadWrite = 0, | ||
| Read = 1 | ||
| //... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this please, are there any other fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may add Access:Write eventually.
Currently Write is not a planned feature, so instead of adding some unsupported Access enum, we omit Write for now.
| } | ||
| ``` | ||
|
|
||
| If a pointer is `Access::Read`, a user program may only read from the given pointer. If a pointer is `Access::ReadWrite`, a user program may read from a given pointer or write to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: may => can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use may only and can only in our documentation.
I do not fully understand why (may is formal, can tends to be informal)
| let N : int, | ||
| let R : CoopMatMatrixUse, | ||
| let matrixLayout : CoopMatMatrixLayout, | ||
| Access access, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make access and addrSpace default as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not really make a difference. A user must supply a Ptr parameter.
|
|
||
| ### Support For Coherent Workgroup Memory | ||
|
|
||
| Any access through a coherent-pointer to a `groupshared` object is coherent; Since Slang does not currently support pointers to `groupshared` memory, this proposal will extend the existing `AddressSpace::GroupShared` implementation for pointers as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since => since.
| The following keyword use is disallowed: | ||
| * `globallycoherent T*` | ||
| * `coherent T*`. | ||
| * `const T*` and `T* const` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and T const*
resolves: #7408
Changes: