jcea has quit [Read error: Connection reset by peer]
jcea1 has joined #videocore
jcea1 is now known as jcea
jcea has quit [Ping timeout: 240 seconds]
gpoo has quit [Ping timeout: 245 seconds]
itoral has joined #videocore
luckyxxl has joined #videocore
luckyxxl has quit [Quit: bye]
luckyxxl has joined #videocore
austriancoder_ has joined #videocore
austriancoder has quit [*.net *.split]
austriancoder_ is now known as austriancoder
<itoral>
apinheiro: about the pipeline cache work, I understand the idea is to try to identify the compiled variant first by only using a sha1 generated from the pCreateInfo alone... but that in order to actually compile the variant we need more infor than that (or rather, I guess, info that is derived from that during nir processing)
<itoral>
is that correct?
<apinheiro[m]1>
itoral:
<apinheiro[m]1>
> only using a sha1 generated from the pCreateInfo alone.
<apinheiro[m]1>
not only the pCreateInfo
<apinheiro[m]1>
that sha1 would include the sha1 from the spir-v shader (or nir if internally created)
<itoral>
ah, ok
<apinheiro[m]1>
so that it is the reason we could identify properly the variant
<apinheiro[m]1>
because anything coming from the nir or their lowerings
<apinheiro[m]1>
are implicit from the spir-v shaders
<itoral>
I was going to ask precisely that
<itoral>
alright, that makes me less concerned
<apinheiro[m]1>
itoral: you can take a look to the method pipeline_hash_variant
<apinheiro[m]1>
that doesn0t appear on the patches as I didn't need to modify it
<apinheiro[m]1>
although now that Im looking at it
<apinheiro[m]1>
it is basically what Im saying now
<apinheiro[m]1>
that hash is created using the spir-v(internal nir) sha1, plus the key filled with the pCreateinfo data
<itoral>
apinheiro[m]1: I think we need to make that very clear in the code, because it is not immediately obvious and anyone who goes through that process is going to be surprised that we do cache lookups with a partial key
<itoral>
and wonder whether that is a bug
<itoral>
I think we probably put a comment explaining this either when we call pipeline_search_for_variant, or at the header of that function (or maybe in both places)
<apinheiro[m]1>
ok, true, I explained it on the commit message, but it is true that it would be more clear if we add it as a comment
<apinheiro[m]1>
itoral: would you mind to point it as part of the review? so as I update the patch I can go iterating on your comments there?
<itoral>
sure
<itoral>
apinheiro[m]1: the commit log also says: "But there are other places that still assume that the nir shader will be always available"
<itoral>
since this patch can produce a pipeline where the stages don't have a nir any more, is that safe?
<itoral>
I guess my question is, is this patch supposed to break anything that is then fixed in follow-up patches?
<apinheiro[m]1>
yes, that patch would break things, that would be fixed in those follow-up patches
<apinheiro[m]1>
let me see
<apinheiro[m]1>
hmm
<apinheiro[m]1>
I think that I could reorder the patches
<apinheiro[m]1>
and have the big one as the last one
<apinheiro[m]1>
and having the others as pre-work for that one, instead of fixes
<apinheiro[m]1>
hmm no
<apinheiro[m]1>
I have just tested, and I get some rebase conflicts
<apinheiro[m]1>
although if you want, I could try to solve them, so as I said, the bit patch would be come the last
<itoral>
I don't think we should be mergeing patches that are known to break stuff, that is a really bad idea
<itoral>
there are two options:
<itoral>
reorganize the patches to avoid that or, if that is difficult, merge patches into one patch
<apinheiro[m]1>
ok, will try the reorganize thing first
<apinheiro[m]1>
as probably that patch will change a little, perhaps you could stop the review
<apinheiro[m]1>
and I will ping you back when it is ready
<itoral>
another thing that makes me a bit uneasy about this series, is that now we are relying on having a partial key to get cache hits. If we ever search for a variant with a complete key we're going to have a cache lookup failure, right?
<apinheiro[m]1>
but we shouldn't do that
<apinheiro[m]1>
perhaps I could add a comment about that
<itoral>
Not saying that this may not pay off, but it kind of feels like a potential can of worms when we then refavctor future code
<apinheiro[m]1>
hmm in fact
<apinheiro[m]1>
now that you mention that
<apinheiro[m]1>
ok, yes, a thing that I was not doing correctly
<apinheiro[m]1>
I have this comment on private.h
<apinheiro[m]1>
well, I have a comment on private.h pointing that the copy of the v3d_key that we maintain there
<apinheiro[m]1>
are those created with pCreateInfo
<apinheiro[m]1>
the idea is using them as base when we go to compile the variant
<apinheiro[m]1>
but I was modifying it
<apinheiro[m]1>
<itoral "Not saying that this may not pay"> what Im going to do is making clear that in order to create the cache, we would use those keys
<apinheiro[m]1>
and when compiling, we will amend a copy of it
<apinheiro[m]1>
itoral: that sounds better?
<itoral>
I guess, I am still a bit concerned that this is still a bit fragile / easy to forget, when reading the code, but I guess it is just a matter to let this new way of doing things settle
<itoral>
We only compile and search for compiled shaders in very specific places, so I guess having the right comments there should make it okay
<apinheiro[m]1>
itoral: other alternative that I though was to keep one populate method
<apinheiro[m]1>
but checking if we have the nir available (of the descriptor maps)
<apinheiro[m]1>
assuming that if they are not available, is because we are looking for the variant
<apinheiro[m]1>
but I saw that also fragile, and would also make the code more messy
<itoral>
yes, it doens't sound like that would be better
<itoral>
apinheiro[m]1: would it be easy to figure out an assertion to check if we are ever doing a cache lookup with a full key?
<itoral>
Maybe testing for non-zero on a field that is expected to be non-zero if the key has been fully populated?
<itoral>
I ask because this kind of thing could lead to performance regressions if we ever mess this up in the future
<itoral>
and they would go unnoticed because we would end up recompiling shaders
<itoral>
so it would not affect functionality
luckyxxl has quit [Quit: bye]
<apinheiro[m]1>
on this times it is a pity that VkGraphicsPipelineCreateInfo is a bunch of linked structs
<apinheiro[m]1>
because if not, the way to solve that would be to use it as part of the variant pipeline cache
<apinheiro[m]1>
itoral: in any case, ok, I will try to think if there is a way to be more sure, or make it more clear, that we are always using a partial cache for the pipeline cache lookup
gpoo has joined #videocore
jcea has joined #videocore
<apinheiro[m]1>
itoral: argh, I have been 30 minutes trying to find what I did wrong when applying your feedback, as I got some regressions,
<apinheiro[m]1>
and what I did wrong was rebasing against master
<apinheiro[m]1>
itoral: so dragons ahead, there are regressions on master, will do a git bisect to check where
<apinheiro[m]1>
*when
<apinheiro[m]1>
it started
<itoral>
ugh
<apinheiro[m]1>
jasuarez: one question about the mesa CI
<apinheiro[m]1>
it uses release or debug builds?
<jasuarez>
let me check
<jasuarez>
I think debug
<jasuarez>
buildtype=debugoptimized
<jasuarez>
but it can be overrided if desired
<apinheiro[m]1>
jasuarez: hmm, strange
<apinheiro[m]1>
because I found the guilty commit:
<apinheiro[m]1>
and I assumed that we didn0t detect anything because we were running on release
<apinheiro[m]1>
ah
<apinheiro[m]1>
debugoptimized
<apinheiro[m]1>
what it is the difference between debugoptimized and release?
<jasuarez>
I think debug optimized has the symbols
<apinheiro[m]1>
ok
<apinheiro[m]1>
<jasuarez "but it can be overrided if desir"> well, I think that this is a kind of special case that will not happen really often
<apinheiro[m]1>
so I guess that we can leave as it is for now
<apinheiro[m]1>
itoral: ^ opinion?
<itoral>
I lean towards leaving it as is for now, it is an exceptional case
<itoral>
With that said, if using a debug build of Mesa doesn't inccur in significant runtime differences, i think there is value in using debug builds
<itoral>
but I guess there should be a notable difference, so based on that assumption, I think it is not worth it
itoral has quit [Quit: Leaving]
<apinheiro[m]1>
fwiw, at least with my experience running full runs, there is a significant difference
<apinheiro[m]1>
not sure when doing on the mesa-ci cts subset