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

[Jose Fonseca]

----- 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 */
};


   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.


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?


Jose