On Sat, 2017-08-05 at 19:46 -0500, Aaron Watry wrote:
> On Fri, Aug 4, 2017 at 1:32 PM, Jan Vesely <jan.ves...@rutgers.edu> wrote:
> > On Sun, 2017-07-30 at 20:26 -0500, Aaron Watry wrote:
> > > According to section 5.8.4.5 of the 2.0 spec, the CL C version is chosen
> > > by:
> > > 1) If you have -cl-std=CL1.1+ use the version specified
> > > 2) If not, use the highest 1.x version that the device supports
> > >
> > > Curiously, there is no valid value for -cl-std=CL1.0
> > >
> > > Signed-off-by: Aaron Watry <awa...@gmail.com>
> > > Cc: Pierre Moreau <pierre.mor...@free.fr>
> > >
> > > v2: (Pierre) Move create_compiler_instance changes to correct patch
> > > to prevent temporary build breakage.
> > > Convert version_str into unsigned and use it to find language version
> > > Add build_error for unknown language version string
> > > Whitespace fixes
> > > ---
> > > .../state_trackers/clover/llvm/invocation.cpp | 61
> > > +++++++++++++++++++++-
> > > 1 file changed, 60 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > index 7c8d0e738d..ca75596b05 100644
> > > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> > > @@ -93,6 +93,65 @@ namespace {
> > > return ctx;
> > > }
> > >
> > > + unsigned get_language_version_from_string(const std::string
> > > &version_str){
> > > + if (version_str == "1.0"){
> > > + return 100;
> > > + }
> > > + if (version_str == "1.1"){
> > > + return 110;
> > > + }
> > > + if (version_str == "1.2"){
> > > + return 120;
> > > + }
> > > + if (version_str == "2.0"){
> > > + return 200;
> > > + }
> > > + throw build_error("Unknown/Unsupported language version");
> > > + }
> >
> > I'm a bit conflicted about this. returning int from device.cl_version()
> > might be nicer, we are using C++ string so we probably don't have to
> > worry about generating new strings all the time.
>
> While the device version could be changed to an integer, we still need
> some string parsing, if we're going to detect the language version
> specified in the build options. I could refactor out a
> get_language_from_version_id(uint) and then use that in
> get_language_from_version_str, which would at least let us keep the
> device clc version as an integer the whole time, instead of needlessly
> treating it as a string and then parsing it later. But I do think
> that we want to keep the string parsing piece, at least for now.
>
> >
> > > +
> > > + clang::LangStandard::Kind
> > > + get_language_from_version_str(const std::string &version_str,
> > > + bool is_opt = false) {
> > > + /**
> > > + * Per CL 2.0 spec, section 5.8.4.5:
> > > + * If it's an option, use the value directly.
> > > + * If it's a device version, clamp to max 1.x version, a.k.a. 1.2
> > > + */
> > > + unsigned version = get_language_version_from_string(version_str);
> > > + if (!is_opt && version > 120 ){
> > > + version = 120;
> > > + }
> > > + switch (version){
> > > + case 100:
> > > + return clang::LangStandard::lang_opencl10;
> > > + case 110:
> > > + return clang::LangStandard::lang_opencl11;
> > > + case 120:
> > > + return clang::LangStandard::lang_opencl12;
> > > + case 200:
> > > + return clang::LangStandard::lang_opencl20;
> > > + default:
> > > + throw build_error("Unknown/Unsupported language version");
> > > + }
> > > + }
> > > +
> > > + clang::LangStandard::Kind
> > > + get_language_version(const std::vector<std::string> &opts,
> > > + const std::string &device_version) {
> > > +
> > > + const std::string search = "-cl-std=CL";
> > > +
> > > + for(auto opt: opts){
> > > + auto pos = opt.find(search);
> > > + if (pos == 0){
> > > + auto ver = opt.substr(pos+search.size());
> > > + return get_language_from_version_str(ver, true);
> > > + }
> > > + }
> >
> > I don't think you need the above. we only set the defaults, so clang
> > should be able to parse this option on its own if we pass it along.
>
> Should... but yet, without setting the language defaults to CL 1.2
> below, any kernel that uses the 'static' keyword fails to compile,
> even with '-cl-std' set in the build options. Only when you invoke
> set_lang_defaults with lang_opencl12 does it actually compile and run
> properly. I guess it clang *could* be getting confused about the
> language version if the program is specified as 1.2, and we are
> explicitly setting the language defaults for 1.1.
>
> I guess I haven't tried to see what happens if you just omit the
> set_lang_defaults call altogether, but I suspect that wouldn't end
> well.
>
> >
> > > +
> > > + return get_language_from_version_str(device_version);
> > > + }
> > > +
> > > std::unique_ptr<clang::CompilerInstance>
> > > create_compiler_instance(const target &target,
> > > const std::vector<std::string> &opts,
> > > @@ -129,7 +188,7 @@ namespace {
> > > compat::set_lang_defaults(c->getInvocation(), c->getLangOpts(),
> > > compat::ik_opencl,
> > > ::llvm::Triple(target.triple),
> > > c->getPreprocessorOpts(),
> > > - clang::LangStandard::lang_opencl11);
> > > + get_language_version(opts,
> > > device_version));
> >
> > I'd imagine this could be something like
> > get_language_from_version(std::max(dev.clc_version(), 120))
>
> But that would also set the default to a minimum of 1.2, while we
> still only claim 1.1 support for all/most devices. I'm not quite read
> to do that (although I'd love to). It's not until we are convinced of
> the API completeness, and at least not regressing anything that I'd be
> happy with doing that. There's a few built-in functions that are still
> missing that were added in 1.2 that we should probably finish up
> before bumping the CLC version (or just make it auto-detect based on
> capabilities and built-in library functionality). popcount would be an
> easy built-in to deal with, printf() not so much.
>
> Also, that max() call would automatically upgrade all programs to CL
> 2.x if the device supports it, which is explicitly not allowed.
>
> 1) If the program specifies cl-std, use exactly that version, as long
> as the device supports it.
> 2) If cl-std is not specified, use min(120, dev.clc_version()).
>
> The above, is basically being calculated by the get_language_version
> call and that for loop in get_language_version.yes, it should have been std::min instead of std::max. Looks like I don't fully understand the point of set_lang_defaults. Is the language correctly set after "clang::CompilerInvocation::CreateFromArgs()"? We pass all the options there. Does passing "LangStandard::lang_unspecified" instead of clc version change the behaviour? If you need to parse the string at least once then we can do it for dev.clc_Verions() as well. Maybe you can use something like "clang::LangStandard::getLangStandardForName()" to do the parsing? sorry for the lack of answers, I need to study the clang invocation API a bit more. I'm just trying to avoid duplicating what clang already implements, and ideally reduce clover handling of the clc language to minimum. thanks, Jan > > --Aaron > > > > > Jan > > > > > > > > c->createDiagnostics(new clang::TextDiagnosticPrinter( > > > *new raw_string_ostream(r_log), -- Jan Vesely <jan.ves...@rutgers.edu>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
