[Mesa-dev] [PATCH 10/11] clover: Add function for building a clover::module for non-TGSI targets

[Francisco Jerez]

Tom Stellard <tstellar at gmail.com> writes:

> On Sun, May 13, 2012 at 04:10:50PM +0200, Francisco Jerez wrote:
>> Tom Stellard <tstellar at gmail.com> writes:
>> 
>> > On Sun, May 13, 2012 at 12:40:43AM +0200, Francisco Jerez wrote:
>> >>[...]
>> >> I can think of two different ways this could work (your solution would
>> >> be somewhere in between):
>> >> 
>> >>  - The r600g LLVM back-end could support some well-defined output object
>> >>    format (e.g. the one implemented by the clover::module classes), like
>> >>    most of the other LLVM back-ends.  You probably want to do this
>> >>    anyway if you want to be able to compile CL programs off-line.  If
>> >>    you do it this way, the state tracker will just call clang to
>> >>    completion using the appropriate target and pass the generated
>> >>    machine code to the pipe driver.
>> >> 
>> >>    If you think supporting different hardware versions with different
>> >>    ISAs would be a problem under this scheme, we could have another
>> >>    compute cap that would determine a specific variant of the ISA.
>> >>
>> >
>> > I think this is the most practical of these two options.  However, the
>> > biggest draw back of this solution is that if a target uses a backend that
>> > is not a part of the LLVM tree (the r600 backend is not in the LLVM tree,
>> > but I'm working on getting it in), the backend would have to be included
>> > as a part of clover and drivers using LLVM for other state tracker would
>> > end up with one copy of their LLVM backend in clover and another in the
>> > driver.
>> 
>> Apparently it's already standard practice in LLVM to build back-ends as
>> separate object files.  I don't see why you couldn't build the r600
>> back-end as a shared library.  There's no reason for it to be duplicated
>> in the source tree or the address space of the process it's running in.
>>
>
> I don't think this will work if clover and the back-end link
> statically to the LLVM libraries, which is what Mesa does by default.
> LLVM relies on global pointers to identify float types within
> a module, so it doesn't work to pass a module between shared libraries that
> have each linked separately with LLVM, which is what will be happening
> if the Clang frontend and the backend live in separate shared objects.
>

I don't think that's going to be a problem.  If you do it this way the
state tracker will be generating r600 machine code directly and there
should be no pointer identifiers being leaked through the gallium API.

But, anyway, I think you want to link to the r600 back-end from the
target instead of linking from the state tracker and pipe driver, that
way you make sure you're only linking once.

>
>> > I guess this might be able to work, but I'm not really sure why doing
>> > it this way would be better than what is implemented in this patch.
>> >
>> I just think it would be useful by itself to have the r600 back-end able
>> to emit machine code in a specific object file format, and at the same
>> time, by doing so you avoid driver-specific code in the state tracker
>> that does one thing or another depending on the hardware it's running
>> on, which seems like a layering violation to me.
>
> State trackers already use caps to do different things depending on
> what hardware it's running.  So, I'm not really sure why using
> PIPE_SHADER_CAP_PREFFERED_IR is any different.
>

I'm aware, but the fact that we're already doing it elsewhere doesn't
mean it's a good practice if it can be avoided.  

If it turns out it can't be avoided, IMHO, any driver caps that affect
the state tracker behaviour should have a well-defined meaning
independent from the pipe driver that is running behind, i.e. a state
tracker should never have to do: "if (r600) { /* do something */ } else
{ /* do something else */ }".

>> 
>> >>  - Another option would be to forget about driver-specific IRs in the
>> >>    compute API.  The pipe driver would have the choice between TGSI and
>> >>    LLVM, if it wants LLVM, the state tracker would do roughly what
>> >>    you're doing here using some sort of "identity" LLVM target that
>> >>    would do nothing but describing the peculiarities of the hardware
>> >>    (e.g. endianness, widths of the supported primitive types), which in
>> >>    turn would be queried from the pipe driver using compute caps.
>> >>
>> >
>> > This is the way I thought it would work initially, but the only way to
>> > describe a target to Clang is by using the target triple, so passing a
>> > list of caps into Clang is not going to work.
>> >
>> Why not?  You're in control of the back-end, maybe I'm missing something
>> but I don't see why the state tracker couldn't provide all the required
>> parameters to the back-end as it's instantiated.
>>
>
> Clang doesn't consult the backend when it converts OpenCL C to LLVM IR,
> so passing extra information to the backend won't help.  We need to pass
> target information directly to Clang and the only way to do this is via
> the target triple.
>
>
>> If this is impossible for some reason, can't we encode these parameters
>> in the processor variant part of the target triple?  Aren't these
>> parameters supposed to be encoded as a target description string at some
>> point anyway?
>>
>
> The target description string is one of the attributes on the TargetInfo
> class in Clang, but there are others as well.  Take a look at the
> Targets.cpp file in Clang and you'll see what I'm talking about:
> http://clang.llvm.org/doxygen/Targets_8cpp-source.html
>
>
>> >> Personally I think the latter would be closer to ideal, but I guess it
>> >> would also involve more work...
>> >
>> > My preference is to keep this patch as is.  At this point, I'm not
>> > really sure why the proposed alternate solutions would be better than
>> > the current implementation, and I think if we want to support out of
>> > tree backends, we will need to at least have the option of doing it this
>> > way.
>> >
>> What bothers me of this solution is that you're pumping bytecode using a
>> generic representation, but in a non-generic way, that isn't going to be
>> useful for drivers other than r600 that want compute programs in the
>> same representation -- at least not without a number of driver-specific
>> ad-hoc changes in both LLVM and the CL state tracker.
>> 
>
> This is exactly what other state trackers do.  They produce a generic
> representation (TGSI) in a non-generic way.  For example, st/mesa might
> produce a TGSI fragment shader that uses indirect addressing for r600g,
> but the TGSI representation of the shader is not generic.  That TGSI
> program will not run on r300g, because that driver does not support
> indirect addressing.

I didn't mean that all the code produced by the state tracker should be
able to run on any hardware, I meant that whatever means you use to
generate that code should be made as reusable for other drivers as
possible.

>> I'm fine with the CL state tracker giving out code in LLVM IR, but, if
>> it does, I really think it makes no sense for it to even know it's
>> running on r600.
>>
> Even though LLVM is a generic representation it's not always possible
> to produce an LLVM bitcode program that is guaranteed to run on every
> target, so if we use LLVM IR it will have to some knowledge about that
> target on which it will executed.
>

Yes, what we are discussing is how to pass this information from the
pipe driver to the compiler.  If you don't want to fix clang, and you
don't like my other proposal either, I'd consider separating the driver
cap that determines the IR from the one that determines the target,
until we have a generic way to pass the target description to the
compiler.

>
>> > Even though I prefer this solution, ultimately I want to come up with
>> > something that works for everyone even if it isn't exactly what I
>> > proposed.  So, I hope we will be able to work something out.
>> >
>> > -Tom
>
> -Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120514/8becc1b5/attachment.pgp>