[Mesa-dev] [RFC][PATCH 00/11] gallium: Basic compute infrastructure.

[Tom Stellard]

On Wed, Apr 18, 2012 at 07:34:15AM -0700, Jose Fonseca wrote:
> 
> 
> ----- Original Message -----
> > On Tue, Apr 17, 2012 at 06:27:20PM +0200, Francisco Jerez wrote:
> > > Jose Fonseca <jfonseca at vmware.com> writes:
> > > 
> > > > Francisco,
> > > >
> > > > Sorry for the delay reviewing this, but I  haven't been able to
> > > > dedicate some time until now.
> > > >
> > > > Overall, it's a great piece of work! Just a few relatively minor
> > > > comments/requests.
> > > >
> > > 
> > > Hi Jose, thanks for the comments.
> > > 
> > > >[...]
> > > >>       gallium/tgsi: Add support for raw resources.
> > > >
> > > > What's the difference between raw resources and buffers?
> > > > Shouldn't all
> > > > buffers be raw, and non-buffers resources non-row?
> > > >
> > > 
> > > The difference is that raw resources have no associated data type,
> > > so
> > > there's no type conversion between what the shader sees and what
> > > ends up
> > > stored in memory.  This is somewhat orthogonal to the number of
> > > addressing dimensions of the resource, so you can get raw 2D
> > > textures,
> > > 3D textures, etc.
> > > 
> > > You're right that CL only uses raw buffers right now, but D3D11
> > > makes a
> > > similar distinction and both raw and non-raw buffers are likely to
> > > be
> > > useful if someone tries to implement ByteAddressBuffers and
> > > Buffers,
> > > respectively.
> > > 
> > > >[...]
> > > >>       gallium/compute: Drop TGSI dependency.
> > > >
> > > > This fine in principle, but I don't understand what
> > > > PIPE_COMPUTE_CAP_IR_TARGET's triplets are supposed to be (a
> > > > string?),
> > > 
> > > Yes, it's supposed to be a null-terminated string containing a
> > > target
> > > triple specification in the "standard" form many compilers
> > > understand,
> > > as documented in "screen.rst".
> > > 
> > > > and how would this scheme work, e.g., if a driver wanted to
> > > > consume
> > > > GLSL IR in the future.
> > > 
> > > Hm, I'm not convinced that letting a driver consume GLSL IR would
> > > be a
> > > good idea in itself.  It opens the door to a situation where each
> > > driver
> > > has to provide a compiler front-end for each individual API it aims
> > > to
> > > support, and it would break with the principle of having
> > > API-agnostic
> > > drivers running under a hardware-agnostic state tracker.
> > 
> > The target triple and the IR type are two separate issues.  The
> > target triple really doesn't say anything about the IR type the
> > driver
> > expects.  Really, the only purpose of the target triple is to
> > describe the target
> > to help the compiler frontend generate correct code.
> > 
> > I think we should also add a query function like this in order to
> > communicate to
> > the state tracker the kind of IR it should pass to the driver:
> > 
> > unsigned get_prefered_ir(unsigned shader_type) {
> >     switch(shader_type) {
> >     default:
> >        return GALLIUM_IR_TGSI;
> >     }
> > }
> > 
> > > 
> > > IMHO, in an ideal world we'd have one common transport IR all
> > > drivers
> > > would be comfortable with, otherwise you get a matrix of code paths
> > > that
> > > is likely to get more and more painful to debug and maintain as the
> > > number of drivers and state trackers grows.
> > > 
> > > As AMD didn't seem to be willing to use TGSI directly in their
> > > compute
> > > stack, the purpose of this change was to simplify the driver code
> > > in
> > > cases where the front-end compiler already happens to support the
> > > native
> > > binary format required by the pipe driver, so, right now
> > > PIPE_COMPUTE_CAP_IR_TARGET can be either "tgsi" or the hardware's
> > > native
> > > binary format.
> > > 
> > > > I think that an enum would be preferrable.
> > > >
> > > I'm OK with changing it to an enum if you think it would be
> > > preferable.
> > > 
> > > What made me decide for a string was that if it were to be an enum,
> > > we
> > > would need to have hardware-specific enum values
> > > (e.g. PIPE_COMPUTE_CAP_IR_TARGET_AMD_R700), and the state trackers
> > > would
> > > need driver-specific code to make a proper interpretation of it.
> > >  If
> > > it's a string in some standard form the state tracker can just pass
> > > it
> > > on to the compiler without looking inside.
> > 
> > I think this makes more sense as a string.  The target triple is a
> > pretty
> > standard format and as Francisco points out an enum would require the
> > state tracker to have driver specific code to determine which triple
> > to use.
> 
> I'm ok with punching LLVM IR through gallium, but I really would prefer adding LLVM specific interfaces. 
> 
> Also, while you say target triple is a pretty standard format, I don't see it standardized anywhere. It looks like even LLVM changes every now and then.
> 
> So both things make thing this is a bad interface.
> 
> So, let's use an enum for now, like
> 
> enum pipe_ir {
>     PIPE_IR_TGSI = 0,
>     PIPE_IR_LLVM,
>     PIPE_IR_LLVM_R700, /* or maybe PIPE_IR_AMD_IL_R700, as I'm not entirely sure what's exactly being punched through */
> };
> 

It's LLVM IR that is being passed.  The enum should look like this:

enum pipe_ir {
    PIPE_IR_TGSI = 0,
    PIPE_IR_LLVM,
    PIPE_IR_LLVM_R600,
    PIPE_IR_LLVM_SI
};
> 
>    enum pipe_preferred_ir ir = screen->get_shader_param(screen, shader, PIPE_SHADER_CAP_PREFERRED_IR);
>    switch( ir) {
>    default:
>    case PIPE_IR_TGSI:
>        /* fallback to TGSI
>        return FALSE;
>    case PIPE_IR_LLVM:
>        target = "???";
>        break;
>    case PIPE_IR_LLVM_R700:
>        target = "???-amd-r700";
>        break;
>    }
>    ...  
> 
> 
> The switch table can be in an inline helper function. 
> 
> And lets move on. I'm happy to revisit this issue later when there are more drivers using LLVM IR.
> 
Sounds good.

> 
> BTW, why does the frontend need to know the target triplet? That is, why isn't the frontend passing machine independent LLVM IR, and let the target specific information be added/processed inside the target driver?
> 
> 

I think it needs to know the triplet to determine things like pointer
size and the width of data types like size_t.

-Tom

> Jose
>