----- Original Message -----
> 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.
... prefer _not_ 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.
Ah yes. I recall this being discussed on LLVM mailing lists -- it looks like
there isn't really a way to make truly target independent IR.
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
