[Mesa-dev] [PATCH 11/11] r600/radeonsi/clover: always assume PIPE_SHADER_IR_NATIVE for clover
Jan Vesely
jan.vesely at rutgers.edu
Wed Feb 7 01:50:45 UTC 2018
On Fri, 2018-02-02 at 10:22 +0100, Pierre Moreau wrote:
> 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.
Why? what is this good for? Is the expected path for radeonsi:
clc->llvm->spirv->nir->llvm->isa, or clc->llvm->spirv->llvm->isa ?
What advantages does it bring to any target? why can't the targets that
are not natively supported by llvm just consume llvm IR and do the
necessary translations on driver side?
> 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.
Why would anyone want that? what is the expected path from CLC? if you
need the external spirv tools package, why not use it in the other
direction and keep LLVM IR as clover preferred representation?
note that libclc is distributed in target specific LLVM IR.
Jan
>
> Pierre
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180206/6fc912c5/attachment.sig>
More information about the mesa-dev
mailing list