Rewrite dependency chapter #1833

krOoze · 2022-04-25T22:19:49Z

williamih · 2022-04-26T01:58:15Z

chapters/synchronization.txt

+criterion of both is "commands occuring later than the first barrier and
+earlier than the second barrier reduced to the operations in
+ename:VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT,
+ename:VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT,


I think this is a really helpful example.

I have one question about this. I could be wrong, but I'm thinking VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT might not be included in the combined selection criteria, because this stage happens before the fragment shader stage - does that seem correct to you?

Copypasta error. I think they are listed this way in the enum, but in the graphics pipe there's just late test inbetween

I'm not keen on this specific example because it leans on the pipeline stage order, which isn't defined yet. I'd rather we highlight a simpler example here to avoid new readers getting confused - e.g. something that includes exactly the same stages directly. We could then elaborate on this example later when pipeline stage order is introduced to get that concept highlighted better (which might be a good improvement regardless).

For us I think this example makes sense, because we've read the spec and are familiar - but for a new reader this would be confusing.

I do think examples might make clear some of the abstract language of scopes and dependencies. So I encourage that.

@Tobski I try to make as brutal example here as possible. I suppose it is sort of a desperate attempt to compensate the lack of sufficiently formal description of what "scope overlap" is...

In my mind the concept is pretty straightforward, so it is mindly annoying that it might probably require like half a page to properly explain in the end. And even so will still probably require an example to have some certainty people grok it correct.

@stonesthrow Yea. I mean a presence of an example (similarly to code comments) indicates that the formal specifying text is lacking. But then again it is better to have crappy specifying language and an example, than have a crappy specifying language and no example...

@krOoze I don't disagree with your intent, but I do think a fresh reader would not be able parse this particular example until they've gone and read a lot more of the chapter. IMO we need something in the style of the Vulkan Guide or a proposal document - where you get a much higher level view of things without all the formality. There you could say a lot of this much more straightforwardly.

In the meantime, I'd rather we simplify this particular example.

@Tobski I was kinda strategically aiming for something hard. This is the where reader needs to slow down. The example demonstrates the original Issue. I suppose I can sacrifice the implicit pipeline stage part of the example for now?

Right, the pipeline stage order is really my main concern here - the rest is fine.

williamih · 2022-04-26T02:07:06Z

chapters/synchronization.txt

+  * Let *Scope~src~* be the first synchronization scope of *S*.
+  * Let *Scope~dst~* be the second synchronization scope of *S*.
+  * Let *Ops~src~* be the subset of operations in *Ops* selected by scope
+    *Src~S~*.


Should this be Scope src not Src S? And similarly for the destination scope below?

Yea.

Going back and forth on those var names. Suggestions?

Hmm - Scope~src makes sense to me. I think that may be better than Src S because in some places in the spec the variable S is used to mean a synchronization command (rather than a scope).

Other people may have more ideas though!

*Scope~src~* is fine, though I'd be tempted to call it *Scope~first~* (and second) to match the wording in the spec.

@Tobski The nomenclature is sorta mixed. The API parameters use src/dst. TBH I like it more because first/second does not convey a difference between the scope in how they are used.

We landed on first/second for some reason that I honestly can't entirely remember. I'm fine with src/dst here, but might be worth considering a nomenclature change for the spec elsewhere to match - though I'd do that in a separate MR because that's going to be churny.

Tobski

I think I'm happy with the direction this is going in, just needs some tightening on various bits of the new language.

Tobski · 2022-04-26T09:52:18Z

chapters/synchronization.txt

 More precisely:

-  * Let *A* and *B* be separate sets of operations.
+  * Let *Ops* be a set of all submitted operations.


Possible issue here is that *S* itself is a set of operations, so would be included in *Ops*. I'm not 100% sure what the ramifications of that might be at the moment...

@Tobski I try to make the scopes do all the work, so if they do not want to include S they just need to say so.

I have not had time to wholistically review stuff yet (hence draft PR), but typically the scope is writen like "all operations recorded before S that operate in stage P", therefore S should never be part of the scope.

I think it might catch other synchronization operations. Which might be a defect from before I think. Should be cleared all the same though. Going to keep this in mind when I update this PR...

Yea that is all in line with my thinking, so that's fine, just something to be aware of.

Possible alternative terms: A is the "prerequisite" operation(s) and B is the "dependent" operation(s).
Synchronization requires the prerequisite to execute before the dependent executes.

@stonesthrow I don't need A and B here at all, because scopes are generally already defined as downselecting to previous or subsequent operations.

Tobski · 2022-04-26T10:02:49Z

chapters/synchronization.txt

+An _execution dependency_ is an implementation's guarantee that for two sets
+of operations, the first set must: _happen-before_ the second set.
+If an operation _happens-before_ another operation, it means the first
+operation must: complete before the second operation is initiated.
 More precisely:


I'm not sure adding "an implementation's" here has the intended effect. My gut says maybe we need to work harder to separate the definition from who guarantees it.

Maybe:

Suggested change

An _execution dependency_ is an implementation's guarantee that for two sets

of operations, the first set must: _happen-before_ the second set.

If an operation _happens-before_ another operation, it means the first

operation must: complete before the second operation is initiated.

More precisely:

An _execution dependency_ defines a relationship between two sets

of operations, such that one set happens-before the other.

Applications can: express execution dependencies via synchronization

commands that require: an implementation to uphold this guarantee,

as defined below:

"happens-before" => "is executed" ? "happens" seems ambiguous.

@stonesthrow I think this is basically first definition of what "happens-before" means, which has very formal meaning. But I think it is otherwisely unfortunately specified only in the extension.

Yea, reading it back it seems this is what we were trying to use as a sort of bootstrapped definition of happens-before, and I don't think it does a good job in light of the memory model definition. "Happens before" is an industry term though (it's on wikipedia!) so I'm kind of inclined to leave it in pending a memory model rewrite that properly introduces the idea later...

OK, if its a understood term. I do understand. But its vague in that "happens" by some unknown entity. Verses this entity does such and such before the next step.

@stonesthrow It's a term defined in the memory_model extension, but it is used througout the specification.

Tobski · 2022-04-26T10:03:22Z

chapters/synchronization.txt

+  * Let *Scope~src~* be the first synchronization scope of *S*.
+  * Let *Scope~dst~* be the second synchronization scope of *S*.
+  * Let *Ops~src~* be the subset of operations in *Ops* selected by scope
+    *Src~S~*.


*Scope~src~* is fine, though I'd be tempted to call it *Scope~first~* (and second) to match the wording in the spec.

chapters/synchronization.txt

Tobski · 2022-04-26T10:13:49Z

chapters/synchronization.txt

+synchronization scope for the purpose od declaring which kinds of operations
+are subject to the dependency introduced by the command.
+Any type of operation that is not selected by the synchronization command's
 synchronization scopes will not be included in the resulting dependency.


Suggested change

synchronization scope for the purpose od declaring which kinds of operations

are subject to the dependency introduced by the command.

Any type of operation that is not selected by the synchronization command's

synchronization scopes will not be included in the resulting dependency.

synchronization scope identifying two sets of operations that will be synchronized

by the command.

Operations that aren't included in these scopes will not be affected by the

command.

I'd rather avoid saying "dependency" in a definition until after the concept has been introduced below (yes I know the text already says it, but we're improving things here 😬)

Dependencies are introduced in the first paragraph of the chapter. For that matter it is the name of the chapter.

By chapter I assume you mean section? And yes, they are, but the concepts themselves are being built up to be explained - they haven't been introduced at this point in the section other than by name. I think this suggestion pairs with the change here or it doesn't entirely make sense.

Tobski · 2022-04-26T10:21:44Z

chapters/synchronization.txt

+criterion of both is "commands occuring later than the first barrier and
+earlier than the second barrier reduced to the operations in
+ename:VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT,
+ename:VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT,


I'm not keen on this specific example because it leans on the pipeline stage order, which isn't defined yet. I'd rather we highlight a simpler example here to avoid new readers getting confused - e.g. something that includes exactly the same stages directly. We could then elaborate on this example later when pipeline stage order is introduced to get that concept highlighted better (which might be a good improvement regardless).

For us I think this example makes sense, because we've read the spec and are familiar - but for a new reader this would be confusing.

Tobski · 2022-04-26T10:30:21Z

chapters/synchronization.txt

+Synchronization operations that formed an _execution dependency chain_ in
+addition to introducing their individual
+<<synchronization-dependencies-execution,execution dependencies>> also
+introduce and additional third execution dependency.
+This additional dependency guarantees that the first dependecy's *Ops~src~*
+_happen-before_ the second dependency's *Ops~dst~*.
+This dependency can: itself also form a chain with another execution
+dependency, which can form an _execution dependency chain_ of more than two
+depenencies.
+_Execution dependency chain_ complements the specification of individual
+execution dependencies with the following description:


This is a bit looser than I'd like, maybe:

Suggested change

Synchronization operations that formed an _execution dependency chain_ in

addition to introducing their individual

<<synchronization-dependencies-execution,execution dependencies>> also

introduce and additional third execution dependency.

This additional dependency guarantees that the first dependecy's *Ops~src~*

_happen-before_ the second dependency's *Ops~dst~*.

This dependency can: itself also form a chain with another execution

dependency, which can form an _execution dependency chain_ of more than two

depenencies.

_Execution dependency chain_ complements the specification of individual

execution dependencies with the following description:

An _execution dependency chain_ implicitly creates another

<<synchronization-dependencies-execution,execution dependency>>.

An execution dependency formed in this way guarantees that *Ops~src~*

in the first dependency in the chain _happens-before_ *Ops~dst~* in the

second dependency in the chain.

This new dependency can: itself become part of another chain, forming an

_execution dependency chain_ of multiple dependencies, up to and including

all operations in the application.

I don't see the difference. Can you point out what the underlying loosness is?

Literally number of words needed to state the same concepts.

Tobski · 2022-04-26T10:33:33Z

chapters/synchronization.txt

+execution dependencies with the following description:
+
+  * Let *S~1~*, *S~2~*, ..., *S~N~* be a list of *N* synchronization
+    commands submitted in this order.


Here you're leaning on submission order, which hasn't been introduced yet. In the execution dependency definition you did an excellent job of getting rid of this, and I think we could repeat that here.

The order of operations is unnecessary if you can formally define the way an execution dependency chain is formed by the intersection of scopes between operations.

Yea it is just the chain of synch dependencies.

Tobski · 2022-04-26T10:34:07Z

chapters/synchronization.txt

+    Any available value remains available until a subsequent write to the
+    same memory location occurs (whether it is made available or not) or the
+    memory is freed.
+


IIRC these spaces lead to unnecessary gaps in the bullet list, as asciidoctor treats them as separate lists, so would like them removed.

Don't worry too much about formatting for now. I will self-review stuff if and when we dedraft this and I will render it and run spellcheck.

Tobski · 2022-04-26T10:35:22Z

chapters/synchronization.txt

+It feels the following is kinda important and should be part of core 1.0
+Vulkan to some extent.
+====
+endif::editing-notes[]


At some point we hope to better integrate the memory model appendix with the synchronization chapter in general, it's just not something we've had the resource to do, yet - I'm not sure an editors note here is necessary as this is tracked internally, but I'm not particularly against including it. For now we've just hidden it behind the extension as some of the definitions only somewhat apply without it.

Mostly just adding some context here, not necessarily suggesting a change.

Particularly the "happens-before\after" is frequently used, so should be integrated with the core spec sooner rather than later...

Tobski · 2022-04-26T10:39:08Z

I've requested two extra reviewers from the WG who had heavy input on the original language here, in case they have any input.

stonesthrow

Just some small comments

stonesthrow · 2022-04-26T14:13:32Z

chapters/synchronization.txt

+An _execution dependency_ is an implementation's guarantee that for two sets
+of operations, the first set must: _happen-before_ the second set.
+If an operation _happens-before_ another operation, it means the first
+operation must: complete before the second operation is initiated.
 More precisely:


"happens-before" => "is executed" ? "happens" seems ambiguous.

stonesthrow · 2022-04-26T14:14:18Z

chapters/synchronization.txt

 <<synchronization-pipeline-stages,pipeline stages>>, which allows other
 pipeline stages to be excluded from a dependency.
-Other scoping options are possible, depending on the particular command.
+But other scope to other kinds of operations is possible depending on the


Line 70, reword.

stonesthrow · 2022-04-26T14:14:32Z

chapters/synchronization.txt

-Any type of operation that is not in a synchronization command's
+A synchronization scope is a set of selection criteria for operations.
+A synchronization command has a first synchronization scope and a second
+synchronization scope for the purpose od declaring which kinds of operations


"od" -> "of"

Don't worry too much about typos for now. I will self-review stuff if and when we dedraft this and I will render it and run spellcheck.

stonesthrow · 2022-04-26T14:18:25Z

chapters/synchronization.txt

+criterion of both is "commands occuring later than the first barrier and
+earlier than the second barrier reduced to the operations in
+ename:VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT,
+ename:VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT,


I do think examples might make clear some of the abstract language of scopes and dependencies. So I encourage that.

Rewrite dependency chapter

9c317a8

krOoze mentioned this pull request Apr 25, 2022

Definition of synchronization scopes as sets #1815

Open

williamih reviewed Apr 26, 2022

View reviewed changes

Tobski requested changes Apr 26, 2022

View reviewed changes

Tobski requested review from janharaldfredriksen-arm and jeffbolznv April 26, 2022 10:37

stonesthrow reviewed Apr 26, 2022

View reviewed changes

krOoze mentioned this pull request May 8, 2022

Tighten synchronization scope specification #1843

Draft

Rewrite dependency chapter #1833

Are you sure you want to change the base?

Rewrite dependency chapter #1833

Conversation

krOoze commented Apr 25, 2022

williamih Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamih Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Tobski commented Apr 26, 2022

stonesthrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamih Apr 26, 2022 •

edited

Loading

krOoze Apr 27, 2022 •

edited

Loading

williamih Apr 26, 2022 •

edited

Loading

krOoze Apr 27, 2022 •

edited

Loading

krOoze Apr 27, 2022 •

edited

Loading

krOoze Apr 29, 2022 •

edited

Loading