Add GLSL_EXT_nontemporal_keyword #275
Conversation
|
|
HildarTheDorf
force-pushed
the
features/nontemporal_attribute
branch
from
92b1921 to
9947682
Compare
|
“Should this keyword be treated like restrict and implicitly stripped when calling a function, or like volatile/coherent where it is an error?” Treat it similar to volatile and coherent. Do not silently strip as this could cause unintentional behavior. |
HildarTheDorf
force-pushed
the
features/nontemporal_attribute
branch
from
677474d to
f5eab1c
Compare
|
There is an inconsistency with some of the replaced language. Let’s have them all use the following: “restrict, coherent, volatile, and nontemporal memory” |
|
The current implementation that was merged into GLSLANG requires the Vulkan memory model. Maybe some question of whether that should also be viable in GLSL450, but this is fine for my needs right now. In any case, this spec extension doesn’t mention anything about that dependency. |
HildarTheDorf
force-pushed
the
features/nontemporal_attribute
branch
from
76eba70 to
1ef87ff
Compare
|
@gnl21 What's the procedure for merging extension PRs? |
|
I think this should probably use the EXT namespace, in which case it can be merged whenever the authors are happy with it. If you want to use the global namespace then that will need to be approved by the working group. I've made one wording suggestion and it would be nice to get the restrictions consistent between the spec and glslang before we merge. |
| qualifiers." with the following: | ||
|
|
||
| When calling user-defined functions, opaque-type variables qualified with | ||
| coherent, volatile, readonly, writeonly, and nontemporal may not be passed |
There was a problem hiding this comment.
| coherent, volatile, readonly, writeonly, and nontemporal may not be passed | |
| coherent, volatile, readonly, writeonly, or nontemporal may not be passed |
or you could use the phrasing "any combination of ... " which is used elsewhere.
There was a problem hiding this comment.
The original language used “and,” so it doesn’t make sense to change that here. If you want to change the base spec, then that’s outside the scope of this extension.
There was a problem hiding this comment.
The quotation above ("Replace the sentence...") doesn't include any conjunction here and the referenced version of the spec seems to say "or".
There was a problem hiding this comment.
I just looked at 4.10 Memory Qualifiers in the spec and it uses “and”
There was a problem hiding this comment.
This quotation is a different paragraph. It's only this one use of "and" that I think is incorrect, the others are fine.
There was a problem hiding this comment.
Oh I see now. Yes that should be corrected.
There was a problem hiding this comment.
The original language quote being replaced in this extension is also incorrect and missing the full language.
| Memory accesses to image variables declared using the `nontemporal` | ||
| qualifier may be compiled to assume their value will not be needed | ||
| again in the near future. This allows the compiler to avoid bringing | ||
| that data unnecessarily into cache. |
There was a problem hiding this comment.
Should probably add here "It is a compile-time error to use the nontemporal qualifier when not generating SPIR-V"
There was a problem hiding this comment.
Not sure I’d do this. Better to just leave it as a hint. There’s nothing wrong with other backend compilers deciding to use this.
One point that was mentioned to me when I brought up to Hildar the lack of Vulkan memory model being specified was that another extension that required it also did not make any mention of it. I haven’t pressed the issue because I honestly don’t think it ought to be limited to just the Vulkan memory model, but since this would involve significantly more work to GLSLANG to build in support for GLSL450, it’s probably not likely to be done right now. I’d prefer it to get done eventually rather than constrain the spec to only be limited to the Vulkan memory model. Instead, as a stopgap, this was merged in: KhronosGroup/glslang#3874 I get the feeling that Hildar is done putting significant work into this. It was volunteer work in the first place. I’m also out of time to put much into this myself. I’m inclined to just leave things as they are right now and merge things as is. |
|
I'm reluctant to merge spec extensions that say something is going to work when it doesn't because I don't think this makes for a good experience for developers who come to use this later. I guess since this isn't guaranteeing anything in the first place it's fine to implement it by just ignoring it. |
|
The GL_KHR_memory_scope_semantics requires the Vulkan memory model but doesn’t specify it, setting the precedent here. That said, I would not oppose an effort to update both to have it mentioned. Entirely up to you. |
|
Thanks. GL_KHR_memory_scope_semantics is essentially the extension for using the Vulkan memory model in GLSL and I don't think it will ever be supported without the memory model. We should probably make that clearer in the extension spec. I'm fine with glslang#3874 for this extension. |
The nontemporal image/memory operand in SPIR-V is not exposed in GLSL. This extension adds it.
I believe this was mentioned by @JebScape inside Khronos earlier today.
Questions:
Should this keyword be treated like restrict and implicitly stripped when calling a function, or like volatile/coherent where it is an error?
Should this be in the EXT namespace or elsewhere?