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 <jfons...@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
