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>

Attachment: 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

Reply via email to