On Mon, May 14, 2012 at 05:19:10PM +0200, Francisco Jerez wrote:
> Tom Stellard <tstel...@gmail.com> writes:
>
> > On Sun, May 13, 2012 at 04:10:50PM +0200, Francisco Jerez wrote:
> >> Tom Stellard <tstel...@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 */ }".
>
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?
> >>
> >> >> - 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.
>
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)
-Tom
> >
> >> > 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
