[Mesa-dev] [PATCH 11/11] r600/radeonsi/clover: always assume PIPE_SHADER_IR_NATIVE for clover

[Pierre Moreau]

On 2018-02-02 — 18:07, Timothy Arceri wrote:
> 
> 
> On 02/02/18 17:21, Timothy Arceri wrote:
> > On 02/02/18 16:38, Jan Vesely wrote:
> > > On Fri, 2018-02-02 at 15:03 +1100, Timothy Arceri wrote:
> > > > When PIPE_SHADER_IR_LLVM existed this query made sense but now it
> > > > always returns PIPE_SHADER_IR_NATIVE. Also it is now conlicting
> > > > with PIPE_SHADER_IR_NIR for compute shaders, so just assume this
> > > > is always PIPE_SHADER_IR_NATIVE for clover.
> > > > 
> > > > This change indirectly enables NIR support for compute shaders
> > > > on radeonsi.
> > > > ---
> > > >   src/gallium/drivers/r600/r600_pipe.c              | 6 +-----
> > > >   src/gallium/drivers/radeonsi/si_get.c             | 3 ---
> > > >   src/gallium/state_trackers/clover/core/device.cpp | 3 +--
> > > >   3 files changed, 2 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/src/gallium/drivers/r600/r600_pipe.c
> > > > b/src/gallium/drivers/r600/r600_pipe.c
> > > > index 6c021e568d..287fe497ca 100644
> > > > --- a/src/gallium/drivers/r600/r600_pipe.c
> > > > +++ b/src/gallium/drivers/r600/r600_pipe.c
> > > > @@ -595,11 +595,7 @@ static int r600_get_shader_param(struct
> > > > pipe_screen* pscreen,
> > > >       case PIPE_SHADER_CAP_MAX_SAMPLER_VIEWS:
> > > >           return 16;
> > > >           case PIPE_SHADER_CAP_PREFERRED_IR:
> > > > -        if (shader == PIPE_SHADER_COMPUTE) {
> > > > -            return PIPE_SHADER_IR_NATIVE;
> > > > -        } else {
> > > > -            return PIPE_SHADER_IR_TGSI;
> > > > -        }
> > > > +        return PIPE_SHADER_IR_TGSI;
> > > >       case PIPE_SHADER_CAP_SUPPORTED_IRS:
> > > >           if (rscreen->b.family >= CHIP_CEDAR)
> > > >               return (1 << PIPE_SHADER_IR_TGSI);
> > > > diff --git a/src/gallium/drivers/radeonsi/si_get.c
> > > > b/src/gallium/drivers/radeonsi/si_get.c
> > > > index 40f4cc267e..46cc190db1 100644
> > > > --- a/src/gallium/drivers/radeonsi/si_get.c
> > > > +++ b/src/gallium/drivers/radeonsi/si_get.c
> > > > @@ -391,9 +391,6 @@ static int si_get_shader_param(struct
> > > > pipe_screen* pscreen,
> > > >           break;
> > > >       case PIPE_SHADER_COMPUTE:
> > > >           switch (param) {
> > > > -        case PIPE_SHADER_CAP_PREFERRED_IR:
> > > > -            return PIPE_SHADER_IR_NATIVE;
> > > > -
> > > >           case PIPE_SHADER_CAP_SUPPORTED_IRS: {
> > > >               int ir = 1 << PIPE_SHADER_IR_NATIVE;
> > > > diff --git a/src/gallium/state_trackers/clover/core/device.cpp
> > > > b/src/gallium/state_trackers/clover/core/device.cpp
> > > > index 9dd7eed3f1..116f0c7604 100644
> > > > --- a/src/gallium/state_trackers/clover/core/device.cpp
> > > > +++ b/src/gallium/state_trackers/clover/core/device.cpp
> > > > @@ -243,8 +243,7 @@ device::vendor_name() const {
> > > >   enum pipe_shader_ir
> > > >   device::ir_format() const {
> > > > -   return (enum pipe_shader_ir) pipe->get_shader_param(
> > > > -      pipe, PIPE_SHADER_COMPUTE, PIPE_SHADER_CAP_PREFERRED_IR);
> > > > +   return PIPE_SHADER_IR_NATIVE;
> > > 
> > > This looks like it forces IR_NATIVE for absolutely everybody. Why
> > > should other devices use IR_NATIVE just because radeonsi wants
> > > experimental NIR for GL?
> > 
> > It's not just about experimental NIR. If say freedreno or any other nir
> > driver was to use clover they would have the same problem.
> 
> Well it would be return PIPE_SHADER_IR_NIR here at least.
> 
> > Also I'm 99% certain that no drivers other than radeonsi and r600 even
> > care what this value is. All other drivers have this set to 
> > PIPE_SHADER_IR_TGSI does that make anymore sense?
> 
> I see no there is indeed a tgsi path for clover. I guess maybe this change
> has more impact than I first thought.

I’ll be removing the tgsi path for clover in my v3 for adding SPIR-V support
series for clover, as that path is unused.

> Happy to here suggestions for solving the current conflict in uses of
> PIPE_SHADER_CAP_PREFERRED_IR.

One option could be to:
* look at the preferred IR
  |-> if clover supports it, use it
  |-> else, check if any IR supported by clover are supported by the driver,
      and pick the first one that works

Also, clover will be switching (experimentally first) to using SPIR-V as the
canonical IR, for all the compiling and linking internally, before translating
the resulting executable to a representation the driver can handle. So once
spirv_to_nir supports OpenCL SPIR-V (which I believe is being worked on by Rob
Clark for freedreno, and Karol Herbst for Nouveau), RadeonSI could start
accepting even OpenCL kernels as NIR.

Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180202/eea2f591/attachment-0001.sig>