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

[Tom Stellard]

On Wed, Apr 18, 2012 at 10:32:53PM -0700, Jose Fonseca wrote:
> ----- Original Message -----
> > Jose Fonseca <jfonseca at vmware.com> writes:
> > 
> > > ----- 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 <jfonseca at vmware.com> writes:
> > >> > > > >[...]
> > >> > > > > 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.
> > >> > 
> > 
> > You're right that it's not a real standard, but, that isn't
> > necessarily
> > a problem, as long as we make explicit in our API the exact format we
> > expect from the pipe driver, i.e. we don't depend on LLVM to give a
> > definition of the target triple syntax.  If LLVM ever decides to
> > change
> > their format, I don't see why the state tracker couldn't convert from
> > one format to another, if the change is deterministic.
> > 
> > I'm thinking that another option would be to replace the IR_TARGET
> > cap
> > with three different string caps.  On the one hand it would have the
> > advantage of making the internal structure of the target
> > specification
> > more obvious and easier to process (say the format used by LLVM ever
> > changes in a non-backwards-compatible way), and on the other hand it
> > would be easier to maintain than an enum with driver-specific values.
> > 
> > >> > 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?
> > >> > 
> > 
> > That's also possible in principle.  I'd be OK with dropping this
> > patch
> > completely if we all agree on a transport IR for compute programs, be
> > it
> > LLVM, TGSI, or whatever.
> 
> I was not suggesting standarding on LLVM IR vs TGSI, but rather saying that when LLVM IR is passed, it would be better that it was target agnostic.
> 
> > >> > 
> > >> 
> > >> 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.
> > >
> > IMHO it would make more sense to add shader params for these kind of
> > things.  Otherwise, I don't think the API would be completely
> > self-contained, the state tracker (or whatever the user of the
> > compute
> > API is) would always depend on something external to know how certain
> > data structures have to be laid out in memory.
> 
> That's a good point. If you can replace the triplet by explicit get_shader_cap queries for size_t / alignment etc then I think that would be the best.
> 
> If not, I'd really prefer a pipe_ir/pipe_shader_ir enum as above, as I really I don't want gallium interface to depend on strings of dubious meaning.
> 

I don't think this will work for clover.  The only way I could find
to pass target information to Clang was to use a Triple that clang
understood.

-Tom

> Jose
>