Nouveau - Feb 2016 - Dealing with opencl kernel parameters in nouveau now that RES support is gone

Hi,

On 18-02-16 17:39, Ilia Mirkin wrote:
> On Thu, Feb 18, 2016 at 9:45 AM, Hans de Goede <hdegoede at
redhat.com> wrote:
>> But this does not seem to be hooked up yet for nouveau.
>
> Samuel has patches. See
> https://cgit.freedesktop.org/~hakzsam/mesa/log/?h=arb_compute_shader_v3
Cool, I will take a look at those.
>> So some questions:
>> -The commit by Samual says:
>>   This introduces TGSI_FILE_MEMORY for shared, global and local memory.
Only
>> shared memory is currently supported.
>>
>>   The commit introduces MEMORY[x] and MEMORY[x],SHARED so in reality it
also
>> introduces
>>   a second option next to shared, so what are we going to use plain
MEMORY[x]
>> for?
>>   I suggest using it for global memory but we need to be in agreement
on
>> this.
>
> That sounds fine to me. However what I had in mind was switching the
> SHARED field into a 2-bit field and making it
>
> 1 = SHARED
> 2 = GLOBAL
> 3 = LOCAL
>
> (since for OpenCL you also need to be able to address local or private
> memory). I sorta wanted Samuel to do it, but since I had no idea where
> you were at, or if you were even still working on this, I figured it
> should be fixed up by the first person who needed it.
Sounds good, only the naming is somewhat unfortunate since opencl uses different
naming. I.e. it has no "shared"

OpenCL has:
-global:  accessible by all worker-groups as well as by the CPU
-const:   read-only global
-local:   shared by worker-items in the same worker-group, not shared between
worker-groups
-private: accessible only to a single worker-item

So how do these map to the TGSI:

 > 1 = SHARED
 > 2 = GLOBAL
 > 3 = LOCAL

?

>
>>
>> -What about kernel input parameters, so far these have been using
RES[32764]
>>   I must admit that I do not understand where the file_index of 32764
comes
>>   from (or where any of the file indexes come from for
>> src/gallium/tests/trivial/compute.c ?
>>   I have the feeling that these are not used at all, and everything
simply
>> goes
>>   to a flat (virtual) memory space, with the params at address 0,
correct ?
>
> It was never particularly well-specified, which was one of the reasons
> it went away. It also didn't map nicely onto the OpenGL model. There
> is a remaining question of how to do addressing in memory... there's
> 40 bits of address space. Should these implicitly be U64
> (dual-component in TGSI) addresses that are passed around? Not sure
> what the OpenCL position on all this is.
So far I've been using U32 for addresses as that is what Francisco's
original
code was using. And this also is what things like the tgsi LOAD instruction
take. If you're doing a LOAD on a 1d buffer then you will use TEMP[#].x to
specify the index, and the way how this currently works with OpenCL is that
clCreateBuffer() will return a cl_mem type which then gets passed into
the kernel as input parameter and gets treated as a pointer by the compiler,
so e.g. global mem gets treated as a single address space even if there
are multiple global buffers and TEMP[#].x contains the value passed in
via cl_mem as start offset for the buffer + the index into the buffer.

So this means that currently we are limited to U32 since TEMP[#].x is
only 32 bits wide. Internally 40 bits addresses can and should probably
be used so that at least the different memory spaces each have the full
32 bits available.

Note that we could fix this by adding some sort of LOAD64 opcode, which
uses TEMP[#].x and TEMP[#].y as address for 1d buffers, I'm not sure
how this would work for 3d buffers though. I foresee the llvm backend
eventually getting a 64 bit mode where it will use 64 bits for all
pointers and use something like a LOAD64 opcode to indicate that
the indexes (which it effectively uses as addresses / pointers)
are 64 bit wide.
> Also it would be highly preferable to avoid using GLOBAL at all in the
> first place, and just use BUFFER[] or IMAGE[] or whatever. However
> after having a look at how OpenCL works, I don't think that's
> possible. But perhaps I missed something?
OpenCL is very C-like and as such uses a flat address space per memory
space, getting around that is going to be very tricky, if possible at
all.
>>   This seems like a nvidia implementation detail though, and we should
IMHO
>> still
>>   implement a specific TGSI register file for accessing kernel input
params,
>>   agreed ?
>
> I believe that kernel input params should be passed in via user
> uniforms. i.e. CONST. It doesn't have to be a separate UBO, it can use
> the user buffer thing, which is basically what it is now.
>
>>
>> If we can agree on the TGSI syntax for all this, then I'm pretty
sure I can
>> hack
>> something together to get things to work with the latest master again,
and
>> then we
>> can use this as a starting point for properly implementing this.
>>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> I must say I was somewhat unpleasantly surprised by the RES breakage,
yes I
>> know
>> some changes were coming but still. In the future when my llvm->tgsi
work is
>> more
>> stable and I'll be pushing it to llvm upstream we really cannot
make
>> breaking tgsi
>> changes like this.
>
> I thought I had sufficiently explained to you that the resource thing
> was going away -- it was never spec'd, and there were no users.
Yes I was aware that changes were coming.
> Perhaps I didn't make it clear enough though -- sorry. The reality is
> that TGSI is a living thing, and things change every so often. I don't
> think people would be comfortable with locking down TGSI in such a
> way.
Hmm, I do think that if we end up using a llvm-ir -> tgsi step in some
cases that we do need to lock TGSI down. Taken the RES thing for example,
it would have been pretty trivial actually to not nuke it, and instead just
add the image support next to it. Note I'm fine with making changes while
we are figuring this out, but once I start pushing the llvm bits to llvm
upstream (which is still months away) we are going to need some sort of
TGSI stability.

Regards,

Hans

On Fri, Feb 19, 2016 at 5:36 AM, Hans de Goede <hdegoede at redhat.com>
wrote:
> Hi,
>
> On 18-02-16 17:39, Ilia Mirkin wrote:
>>
>> On Thu, Feb 18, 2016 at 9:45 AM, Hans de Goede <hdegoede at
redhat.com>
>> wrote:
>>>
>>> But this does not seem to be hooked up yet for nouveau.
>>
>>
>> Samuel has patches. See
>> https://cgit.freedesktop.org/~hakzsam/mesa/log/?h=arb_compute_shader_v3
>
>
> Cool, I will take a look at those.
>
>>> So some questions:
>>> -The commit by Samual says:
>>>   This introduces TGSI_FILE_MEMORY for shared, global and local
memory.
>>> Only
>>> shared memory is currently supported.
>>>
>>>   The commit introduces MEMORY[x] and MEMORY[x],SHARED so in
reality it
>>> also
>>> introduces
>>>   a second option next to shared, so what are we going to use plain
>>> MEMORY[x]
>>> for?
>>>   I suggest using it for global memory but we need to be in
agreement on
>>> this.
>>
>>
>> That sounds fine to me. However what I had in mind was switching the
>> SHARED field into a 2-bit field and making it
>>
>> 1 = SHARED
>> 2 = GLOBAL
>> 3 = LOCAL
>>
>> (since for OpenCL you also need to be able to address local or private
>> memory). I sorta wanted Samuel to do it, but since I had no idea where
>> you were at, or if you were even still working on this, I figured it
>> should be fixed up by the first person who needed it.
>
>
> Sounds good, only the naming is somewhat unfortunate since opencl uses
> different
> naming. I.e. it has no "shared"
Sad. Well, "shared" is what OpenGL compute shaders use, which is why I
proposed it.
>
> OpenCL has:
> -global:  accessible by all worker-groups as well as by the CPU
> -const:   read-only global
> -local:   shared by worker-items in the same worker-group, not shared
> between worker-groups
> -private: accessible only to a single worker-item
>
> So how do these map to the TGSI:
>
>> 1 = SHARED
>> 2 = GLOBAL
>> 3 = LOCAL
OpenCL global = TGSI global
OpenCL const = TGSI global
OpenCL local = TGSI shared
OpenCL private = TGSI local

Not sure what the distinction is between OpenCL const and global is.
If the const stuff is actually just user-supplied uniforms (and
doesn't need to be in a particular place in memory), then those should
go into TGSI CONST somehow.
>>
>>>
>>> -What about kernel input parameters, so far these have been using
>>> RES[32764]
>>>   I must admit that I do not understand where the file_index of
32764
>>> comes
>>>   from (or where any of the file indexes come from for
>>> src/gallium/tests/trivial/compute.c ?
>>>   I have the feeling that these are not used at all, and everything
>>> simply
>>> goes
>>>   to a flat (virtual) memory space, with the params at address 0,
correct
>>> ?
>>
>>
>> It was never particularly well-specified, which was one of the reasons
>> it went away. It also didn't map nicely onto the OpenGL model.
There
>> is a remaining question of how to do addressing in memory...
there's
>> 40 bits of address space. Should these implicitly be U64
>> (dual-component in TGSI) addresses that are passed around? Not sure
>> what the OpenCL position on all this is.
>
>
> So far I've been using U32 for addresses as that is what
Francisco's
> original
> code was using. And this also is what things like the tgsi LOAD instruction
> take. If you're doing a LOAD on a 1d buffer then you will use TEMP[#].x
to
> specify the index, and the way how this currently works with OpenCL is that
> clCreateBuffer() will return a cl_mem type which then gets passed into
> the kernel as input parameter and gets treated as a pointer by the
compiler,
> so e.g. global mem gets treated as a single address space even if there
> are multiple global buffers and TEMP[#].x contains the value passed in
> via cl_mem as start offset for the buffer + the index into the buffer.
>
> So this means that currently we are limited to U32 since TEMP[#].x is
> only 32 bits wide. Internally 40 bits addresses can and should probably
> be used so that at least the different memory spaces each have the full
> 32 bits available.
>
> Note that we could fix this by adding some sort of LOAD64 opcode, which
> uses TEMP[#].x and TEMP[#].y as address for 1d buffers, I'm not sure
> how this would work for 3d buffers though. I foresee the llvm backend
> eventually getting a 64 bit mode where it will use 64 bits for all
> pointers and use something like a LOAD64 opcode to indicate that
> the indexes (which it effectively uses as addresses / pointers)
> are 64 bit wide.
Well, this LOAD64/STORE64 would just only be defined for MEMORY[]
src/dest, so you don't need to worry about 3d or anything like that. I
believe this is a good solution to the problem.
>
>> Also it would be highly preferable to avoid using GLOBAL at all in the
>> first place, and just use BUFFER[] or IMAGE[] or whatever. However
>> after having a look at how OpenCL works, I don't think that's
>> possible. But perhaps I missed something?
>
>
> OpenCL is very C-like and as such uses a flat address space per memory
> space, getting around that is going to be very tricky, if possible at
> all.
That was my assessment as well.
>
>> Perhaps I didn't make it clear enough though -- sorry. The reality
is
>> that TGSI is a living thing, and things change every so often. I
don't
>> think people would be comfortable with locking down TGSI in such a
>> way.
>
>
> Hmm, I do think that if we end up using a llvm-ir -> tgsi step in some
> cases that we do need to lock TGSI down. Taken the RES thing for example,
> it would have been pretty trivial actually to not nuke it, and instead just
> add the image support next to it. Note I'm fine with making changes
while
> we are figuring this out, but once I start pushing the llvm bits to llvm
> upstream (which is still months away) we are going to need some sort of
> TGSI stability.
I can't unilaterally make such promises. This would have to be
discussed and approved by the other gallium/tgsi stakeholders.

  -ilia

Hi,

On 19-02-16 20:43, Ilia Mirkin wrote:
> On Fri, Feb 19, 2016 at 5:36 AM, Hans de Goede <hdegoede at
redhat.com> wrote:
>> Hi,
>>
>> On 18-02-16 17:39, Ilia Mirkin wrote:
>>>
>>> On Thu, Feb 18, 2016 at 9:45 AM, Hans de Goede <hdegoede at
redhat.com>
>>> wrote:
>>>>
>>>> But this does not seem to be hooked up yet for nouveau.
>>>
>>>
>>> Samuel has patches. See
>>>
https://cgit.freedesktop.org/~hakzsam/mesa/log/?h=arb_compute_shader_v3
>>
>>
>> Cool, I will take a look at those.
>>
>>>> So some questions:
>>>> -The commit by Samual says:
>>>>    This introduces TGSI_FILE_MEMORY for shared, global and
local memory.
>>>> Only
>>>> shared memory is currently supported.
>>>>
>>>>    The commit introduces MEMORY[x] and MEMORY[x],SHARED so in
reality it
>>>> also
>>>> introduces
>>>>    a second option next to shared, so what are we going to use
plain
>>>> MEMORY[x]
>>>> for?
>>>>    I suggest using it for global memory but we need to be in
agreement on
>>>> this.
>>>
>>>
>>> That sounds fine to me. However what I had in mind was switching
the
>>> SHARED field into a 2-bit field and making it
>>>
>>> 1 = SHARED
>>> 2 = GLOBAL
>>> 3 = LOCAL
>>>
>>> (since for OpenCL you also need to be able to address local or
private
>>> memory). I sorta wanted Samuel to do it, but since I had no idea
where
>>> you were at, or if you were even still working on this, I figured
it
>>> should be fixed up by the first person who needed it.
>>
>>
>> Sounds good, only the naming is somewhat unfortunate since opencl uses
>> different
>> naming. I.e. it has no "shared"
>
> Sad. Well, "shared" is what OpenGL compute shaders use, which is
why I
> proposed it.
>
>>
>> OpenCL has:
>> -global:  accessible by all worker-groups as well as by the CPU
>> -const:   read-only global
>> -local:   shared by worker-items in the same worker-group, not shared
>> between worker-groups
>> -private: accessible only to a single worker-item
>>
>> So how do these map to the TGSI:
>>
>>> 1 = SHARED
>>> 2 = GLOBAL
>>> 3 = LOCAL
>
> OpenCL global = TGSI global
> OpenCL const = TGSI global
> OpenCL local = TGSI shared
> OpenCL private = TGSI local
>
> Not sure what the distinction is between OpenCL const and global is.
> If the const stuff is actually just user-supplied uniforms (and
> doesn't need to be in a particular place in memory), then those should
> go into TGSI CONST somehow.
AFAIK OpenCL const is really read-only global, so the data is filled
in by the CPU, then passed to the opencl-kernel running on the GPU
where all worker-items have access to it. I think that TGSI CONST might
indeed be usable for this, but it is probably easiest to treat
it as GLOBAL for now.
>>>> -What about kernel input parameters, so far these have been
using
>>>> RES[32764]
>>>>    I must admit that I do not understand where the file_index
of 32764
>>>> comes
>>>>    from (or where any of the file indexes come from for
>>>> src/gallium/tests/trivial/compute.c ?
>>>>    I have the feeling that these are not used at all, and
everything
>>>> simply
>>>> goes
>>>>    to a flat (virtual) memory space, with the params at address
0, correct
>>>> ?
>>>
>>>
>>> It was never particularly well-specified, which was one of the
reasons
>>> it went away. It also didn't map nicely onto the OpenGL model.
There
>>> is a remaining question of how to do addressing in memory...
there's
>>> 40 bits of address space. Should these implicitly be U64
>>> (dual-component in TGSI) addresses that are passed around? Not sure
>>> what the OpenCL position on all this is.
>>
>>
>> So far I've been using U32 for addresses as that is what
Francisco's
>> original
>> code was using. And this also is what things like the tgsi LOAD
instruction
>> take. If you're doing a LOAD on a 1d buffer then you will use
TEMP[#].x to
>> specify the index, and the way how this currently works with OpenCL is
that
>> clCreateBuffer() will return a cl_mem type which then gets passed into
>> the kernel as input parameter and gets treated as a pointer by the
compiler,
>> so e.g. global mem gets treated as a single address space even if there
>> are multiple global buffers and TEMP[#].x contains the value passed in
>> via cl_mem as start offset for the buffer + the index into the buffer.
>>
>> So this means that currently we are limited to U32 since TEMP[#].x is
>> only 32 bits wide. Internally 40 bits addresses can and should probably
>> be used so that at least the different memory spaces each have the full
>> 32 bits available.
>>
>> Note that we could fix this by adding some sort of LOAD64 opcode, which
>> uses TEMP[#].x and TEMP[#].y as address for 1d buffers, I'm not
sure
>> how this would work for 3d buffers though. I foresee the llvm backend
>> eventually getting a 64 bit mode where it will use 64 bits for all
>> pointers and use something like a LOAD64 opcode to indicate that
>> the indexes (which it effectively uses as addresses / pointers)
>> are 64 bit wide.
>
> Well, this LOAD64/STORE64 would just only be defined for MEMORY[]
> src/dest, so you don't need to worry about 3d or anything like that. I
> believe this is a good solution to the problem.
Right for MEMORY this will work fine. I've no clue yet how images will
work with OpenCL though, hopefully we can avoid the one flat address
space thing there, but I simply don't know yet.
>>> Also it would be highly preferable to avoid using GLOBAL at all in
the
>>> first place, and just use BUFFER[] or IMAGE[] or whatever. However
>>> after having a look at how OpenCL works, I don't think
that's
>>> possible. But perhaps I missed something?
>>
>>
>> OpenCL is very C-like and as such uses a flat address space per memory
>> space, getting around that is going to be very tricky, if possible at
>> all.
>
> That was my assessment as well.
>
>>
>>> Perhaps I didn't make it clear enough though -- sorry. The
reality is
>>> that TGSI is a living thing, and things change every so often. I
don't
>>> think people would be comfortable with locking down TGSI in such a
>>> way.
>>
>>
>> Hmm, I do think that if we end up using a llvm-ir -> tgsi step in
some
>> cases that we do need to lock TGSI down. Taken the RES thing for
example,
>> it would have been pretty trivial actually to not nuke it, and instead
just
>> add the image support next to it. Note I'm fine with making changes
while
>> we are figuring this out, but once I start pushing the llvm bits to
llvm
>> upstream (which is still months away) we are going to need some sort of
>> TGSI stability.
>
> I can't unilaterally make such promises. This would have to be
> discussed and approved by the other gallium/tgsi stakeholders.
Understood.

So back to the problem of getting OpenCL(ish) code to work again with
the recent mesa changes. For starters I would like to get:

src/gallium/tests/trivial/compute.c and then the test with mask 8,
test_input_global() to work again, when that is working I should be
able to adjust my llvm work (and if necessary clover) to start to
work again.

Currently the test_input_global() test uses the following bit of
TGSI code:

COMP
DCL SV[0], THREAD_ID[0]
DCL TEMP[0], LOCAL
DCL TEMP[1], LOCAL
IMM UINT32 { 8, 0, 0, 0 }

    BGNSUB\n"
        UMUL TEMP[0], SV[0], IMM[0]
        LOAD TEMP[1].xy, RINPUT, TEMP[0]
        LOAD TEMP[0].x, RGLOBAL, TEMP[1].yyyy
        UADD TEMP[1].x, TEMP[0], -TEMP[1]
        STORE RGLOBAL.x, TEMP[1].yyyy, TEMP[1]
        RET
    ENDSUB


Where by RINPUT and RGLOBAL get replaces by processing the
code with cpp and the following defines:

#define RGLOBAL        RES[32767]
#define RLOCAL         RES[32766]
#define RPRIVATE       RES[32765]
#define RINPUT         RES[32764]

If I understand how memory is supposed to work, then I would need to
change the TGSI as follows:

COMP
DCL SV[0], THREAD_ID[0]
DCL MEMORY[0]
DCL TEMP[0], LOCAL
DCL TEMP[1], LOCAL
IMM UINT32 { 8, 0, 0, 0 }

BGNSUB\n"
UMUL TEMP[0], SV[0], IMM[0]
LOAD TEMP[1].xy, RINPUT, TEMP[0]
LOAD TEMP[0].x, MEMORY[0], TEMP[1].yyyy
UADD TEMP[1].x, TEMP[0], -TEMP[1]
STORE MEMORY[0].x, TEMP[1].yyyy, TEMP[1]
RET
ENDSUB

This assumes, that as discussed declaring memory without a , SHARED or other
flag means the memory is global.

So 2 questions:

1) Do the above changes for using the new MEMORY keyword look as intended
to you?

2) This only solves the accessing of the global memory, it does not solve
getting to the kernel input kernel parameters, how would I deal with those ?

Currently the kernel input parameters are uploaded by
src/gallium/drivers/nouveau/nv50/nv50_compute.c: nv50_compute_upload_input()

Are we going to change how these parameters get uploaded ? And if not how do we
get to these parameters from TGSI now that we no longer have RES[#] to do so ?

I think we should also keep in mind that what works for nouveau may not work
for other GPU-s, so we should either introduce a MEMORY[x], INPUT or
a new DCL INPUT[0] for this, so that the code for other GPU-s can have its
own handling for this.

Regards,

Hans