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

[Francisco Jerez]

Tom Stellard <thomas.stellard at amd.com> writes:

> On Mon, May 14, 2012 at 05:19:10PM +0200, Francisco Jerez wrote:
>> 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:
>> >>[...]
>> >> > 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 */ }".
>>
>
> I agree with you here.  Just so I'm clear, when you say you don't want
> "if (r600) { /* do something */ } else { /* do something else */ }"
> are you referring to this block of code:
>
> module
> clover::compile_program_llvm(const compat::string &source,
>                              const compat::string &target) {
>
>    if (target == compat::string("TGSI")) {
> #if 0
>       build_binary(source, target, "cl_input");
>       module m = load_binary("cl_input.o");
>       std::remove("cl_input.o");
>       return m;
> #else
>       return module();
>    } else {
>       return build_module_llvm(source, target, "cl_input");
>    }
> #endif
> }
>
> If so, would it be better if this decision was based on the value of
> PIPE_SHADER_CAP_PREFFERED_IR?
>
Well yeah but I would rather not have that conditional at all, for the
reasons I've already mentioned.

>[...]
>> >> 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.
>>
>
> I agree that having separate caps for the IR and the target is a good
> idea.  This is what I was proposing in the previous discussion about
> this http://lists.freedesktop.org/archives/mesa-dev/2012-April/020959.html
>
> Maybe we can revisit this discussion.  What do you think about limiting
> the values of PIPE_SHADER_CAP_PREFFERED_IR to {TGSI, LLVM, NATIVE} and
> then adding a cap like PIPE_SHADER_CAP_LLVM_TARGET that returns the name
> of the target (e.g. r600 for r600g)
>
I think that's definitely a better plan than having the state tracker
take one path or another depending on the target.

> -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/ef2d589c/attachment.pgp>