[Libclc-dev] clang SVN r303370 broke libclc
Aaron Watry via Libclc-dev
libclc-dev at lists.llvm.org
Tue Jul 4 12:22:17 PDT 2017
On Tue, Jul 4, 2017 at 12:25 PM, Liu, Yaxun (Sam) <Yaxun.Liu at amd.com> wrote:
> The patch for AMDGPUTargetInfo is here https://reviews.llvm.org/D34987
>
I gave that patch a spin on both my r600 and GCN-generation cards. The
address-space mapping regression is resolved for both to the best of
my knowledge. My test case was to run the CL CTS atomic tests which
use local kernel arguments for the local atomic function tests. The
atomic local tests all failed before. The regressions are now resolved
(there are still a few unrelated test failures)
I suspect that it would be a good thing for us to fix up our clang
front-end creation code in invocation.cpp anyway, as I believe that it
is the root problem of a few issues that I've run into with clover in
the past as well (e.g. -cl-std not being respected).
--Aaron
> Sam
>
> -----Original Message-----
> From: Liu, Yaxun (Sam)
> Sent: Tuesday, July 04, 2017 11:45 AM
> To: 'Aaron Watry' <awatry at gmail.com>
> Cc: Jan Vesely <jan.vesely at rutgers.edu>; Michel Dänzer <michel at daenzer.net>; libclc-dev at lists.llvm.org
> Subject: RE: [Libclc-dev] clang SVN r303370 broke libclc
>
> Hi Aaron,
>
> In mesa/src/gallium/state_trackers/clover/llvm/invocation.cpp line 97:
>
> std::unique_ptr<clang::CompilerInstance>
> create_compiler_instance(const target &target,
> const std::vector<std::string> &opts,
> std::string &r_log) {
> std::unique_ptr<clang::CompilerInstance> c { new clang::CompilerInstance };
> clang::TextDiagnosticBuffer *diag_buffer = new clang::TextDiagnosticBuffer;
> clang::DiagnosticsEngine diag { new clang::DiagnosticIDs,
> new clang::DiagnosticOptions, diag_buffer };
>
> // Parse the compiler options. A file name should be present at the end
> // and must have the .cl extension in order for the CompilerInvocation
> // class to recognize it as an OpenCL source file.
> const std::vector<const char *> copts =
> map(std::mem_fn(&std::string::c_str), opts);
>
> if (!clang::CompilerInvocation::CreateFromArgs(
> c->getInvocation(), copts.data(), copts.data() + copts.size(), diag))
> throw invalid_build_options_error();
>
> diag_buffer->FlushDiagnostics(diag);
> if (diag.hasErrorOccurred())
> throw invalid_build_options_error();
>
> c->getTargetOpts().CPU = target.cpu;
> c->getTargetOpts().Triple = target.triple;
> c->getLangOpts().NoBuiltin = true;
>
> // This is a workaround for a Clang bug which causes the number
> // of warnings and errors to be printed to stderr.
> // http://www.llvm.org/bugs/show_bug.cgi?id=19735
> c->getDiagnosticOpts().ShowCarets = false;
>
> compat::set_lang_defaults(c->getInvocation(), c->getLangOpts(),
> compat::ik_opencl, ::llvm::Triple(target.triple),
> c->getPreprocessorOpts(),
> clang::LangStandard::lang_opencl11);
>
> c->createDiagnostics(new clang::TextDiagnosticPrinter(
> *new raw_string_ostream(r_log),
> &c->getDiagnosticOpts(), true));
>
> c->setTarget(clang::TargetInfo::CreateTargetInfo(
> c->getDiagnostics(), c->getInvocation().TargetOpts));
>
> return c;
> }
>
> This function calls clang::CompilerInvocation::CreateFromArgs first then set the cpu and target triple. This may not work for certain situations. Because clang::CompilerInvocation::CreateFromArgs assumes target triple and cpu are part of the given arguments and parses them very early, since parsing other arguments may depend on the target triple and cpu. For example, TargetInfo::adjust(LangOptions&) is called during calling of clang::CompilerInvocation::CreateFromArgs. If triple and cpu are not part of arguments, clang will just assumes the default triple and cpu and parses the language options. Later on, when you set the true triple and cpu through c->setTarget, AMDGPUTargetInfo::adjust is not called, therefor the address space mapping in AMDGPUTargetInfo is not correct.
>
> A proper fix of this issue would be passing triple and cpu as arguments to clang::CompilerInvocation::CreateFromArgs instead of setting them later.
>
> About the new address space for global and local: finally global will not change, local will be changed from 3 to 2. If you have library functions implemented with hard coded address space number, you need to change that.
>
> Thanks.
>
> Sam
>
> -----Original Message-----
> From: Aaron Watry [mailto:awatry at gmail.com]
> Sent: Tuesday, July 04, 2017 10:43 AM
> To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>
> Cc: Jan Vesely <jan.vesely at rutgers.edu>; Michel Dänzer <michel at daenzer.net>; libclc-dev at lists.llvm.org
> Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>
> Between Jan and I we should be able to manage testing out the change for both r600/amdgpu back-ends. Feel free to give fiji a test, of course.
>
> If there's something that we can do to specify the kernel language to get the existing code to populate the address space map as well, I wouldn't mind knowing. It probably means that we're missing something else somewhere as well (e.g. I'm not entirely certain whether clang compiler options are currently plumbed through correctly from clover for things like '-cl-std=xx').
>
> Also, if I'm reading the existing code correctly, the global/local address spaces stay the same for amdgpu as 1 and 3? If not, we've got a bit of work to do in libclc, since those are the only address spaces numbers that currently have the atomic functions implemented (which are then mapped by the r600/amdgpu targets. It's not the end of the world, but it'd be nice to know if I should get started on that as well.
>
> On Tue, Jul 4, 2017 at 9:04 AM, Liu, Yaxun (Sam) <Yaxun.Liu at amd.com> wrote:
>> Hi Jan,
>>
>> Private-is-zero means the OpenCL private address space is mapped to address space 0 in LLVM IR. This is what amdgpu used to do. However we found that this does not work for C++-based kernel languages, so we are migrating amdgpu backend to a new address space mapping where OpenCL private address space (also the alloca address space for all languages in general) is mapped to address space 5 (alloca address space) in IR whereas the OpenCL 2.0 generic address space and default address space of other languages to 0 (flat address space) in IR. This change should be transparent to mesa/clover since the ISA should not change.
>>
>> Thanks for your reply about target triple used by mesa. I will let you know once the patch is available. I can do some test on amdgcn(fiji).
>>
>> Sam
>>
>> -----Original Message-----
>> From: Jan Vesely [mailto:jv356 at scarletmail.rutgers.edu] On Behalf Of
>> Jan Vesely
>> Sent: Tuesday, July 04, 2017 9:44 AM
>> To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Aaron Watry
>> <awatry at gmail.com>
>> Cc: Michel Dänzer <michel at daenzer.net>; libclc-dev at lists.llvm.org
>> Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>>
>> On Tue, 2017-07-04 at 02:48 +0000, Liu, Yaxun (Sam) wrote:
>>> What are the target triples used by mesa/clover for amdgpu targets?
>>
>> clover uses 'r600--' and 'amdgcn-mesa-mesa3d'.
>>
>>>
>>> Currently amdgpu targets support 4 address space mappings:
>>>
>>> OpenCL/private-is-zero
>>> Non-OpenCL/private-is-zero
>>> OpenCL/generic-is-zero
>>> Non-OpenCL/generic-is-zero
>>>
>>> The future plan is:
>>> 1. deprecate private-is-zero address space mappings and only support
>>> generic-is-zero address space mappings
>>
>> just out of curiosity, what's the use of private-is-zero mappings?
>> clover shouldn't really care (or is easily switched).
>>
>>> 2. merge OpenCL and Non-OpenCL address space mapping so that it no
>>> longer depends on language
>>>
>>> It seems mesa/clover only uses OpenCL/private-is-zero address space
>>> mapping. If that is the case, I can initialize AddrSpaceMap in
>>> AMDGPUTargetInfo based on target triple and skip the adjustment by
>>> Language options for mesa/clover. Then mesa/clover itself does not
>>> need any change. This should still work after the planned two changes
>>> in the future.
>>
>> thanks, that sounds good to me. let me know if there is a patch to test. I can test r600 (turks), my carrizo machine is stuck on llvm4.0 because of ROCm.
>>
>> Jan
>>
>>>
>>> Sam
>>>
>>> -----Original Message-----
>>> From: Aaron Watry [mailto:awatry at gmail.com]
>>> Sent: Monday, July 03, 2017 4:55 PM
>>> To: Jan Vesely <jan.vesely at rutgers.edu>
>>> Cc: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Michel Dänzer
>>> <michel at daenzer.net>; libclc-dev at lists.llvm.org
>>> Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>>>
>>> On Mon, Jul 3, 2017 at 2:08 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
>>> > On Mon, 2017-07-03 at 00:23 +0000, Liu, Yaxun (Sam) wrote:
>>> > > There are some recent changes about address space mapping in
>>> > > Clang for amdgcn target for better support C++ based kernel languages.
>>> > >
>>> > > Currently, the constructor of AMDGPUTargetInfo does not
>>> > > initialize AddrSpaceMap. Instead, it is initialized by virtual
>>> > > member adjust(LangOptions &Opts) since the address space mapping
>>> > > of amdgcn target depends on language.
>>> > >
>>> > > For backward compatibility, I can let constructor of
>>> > > AMDGPUTargetInfo initialize AddrSpaceMap to the old value.
>>> > > Hopefully this can fix the regression.
>>> >
>>> > Hi,
>>> >
>>> > what's the expected situation going forward? will there be one
>>> > addrspacemap for all languages, or per language maps (set using
>>> > adjust())?
>>> > it'd be nice if we could adapt clover now and future proof it
>>> > before llvm 5 comes out.
>>>
>>> Agreed. I wouldn't mind a fix to LLVM/AMDGPU that preserves backwards compatibility, but if there's something that we can do to mesa/clover to adapt/fix our usage of llvm, I'm interested in hearing about it.
>>>
>>> --Aaron
>>>
>>> >
>>> > Jan
>>> >
>>> > >
>>> > > Thanks.
>>> > >
>>> > > Sam
>>> > >
>>> > > -----Original Message-----
>>> > > From: Aaron Watry [mailto:awatry at gmail.com]
>>> > > Sent: Saturday, July 01, 2017 12:48 PM
>>> > > To: Jan Vesely <jan.vesely at rutgers.edu>
>>> > > Cc: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Michel Dänzer
>>> > > <michel at daenzer.net>; libclc-dev at lists.llvm.org
>>> > > Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>>> > >
>>> > > On Fri, Jun 30, 2017 at 11:10 AM, Jan Vesely via Libclc-dev <libclc-dev at lists.llvm.org> wrote:
>>> > > > On Wed, 2017-06-28 at 22:04 +0000, Liu, Yaxun (Sam) wrote:
>>> > > > > Hi Jan,
>>> > > > >
>>> > > > > What's the command to generate html like
>>> > > > > http://paul.rutgers.edu/~jv3 56/piglit/radeon-latest-5/problems.html with accumulated results?
>>> > > > >
>>> > > > > I used ./piglit summary html summary/turks_cl
>>> > > > > results/turks_cl_* but it only generates html with one
>>> > > > > result, which makes comparison of different runs difficult.
>>> > > >
>>> > > > there needs to be multiple directories with results in 'results/'
>>> > > > directory.
>>> > > > another problem might be that piglit does not overwrite
>>> > > > existing summaries (you should see an error message when that
>>> > > > happens) so you might be stuck with the first one you generated.
>>> > > > ./piglit summary html summary/out results/* --overwrite
>>> > > >
>>> > > > >
>>> > > > > BTW I suspect the regression was due to
>>> > > > > mesa/src/gallium/state_trackers/clover/llvm/codegen/common.cp
>>> > > > > p
>>> > > > > ,
>>> > > > > line
>>> > > > > 131:
>>> > > > >
>>> > > > > if (address_space ==
>>> > > > > address_spaces[clang::LangAS::opencl_local
>>> > > > > -
>>> > > > > compat::lang_as_offset]) {
>>> > > > >
>>> > > > > After my change, this is no longer valid way to get target
>>> > > > > address space. Should be:
>>> > > > >
>>> > > > > if (address_space ==
>>> > > > > address_spaces[clang::LangAS::opencl_local]) {
>>> > > >
>>> > > > compat::lang_as_offset is 0 for llvm >= 5 so those two
>>> > > > statements should be identical. Am I missing something?
>>> > > >
>>> > >
>>> > > Now that I finally got my debugger setup working again, I can say
>>> > > that 'address_spaces', which is fetched by
>>> > > c.getTarget().getAddressSpaceMap() in
>>> > > common.cpp:make_kernel_args, is an empty collection which points
>>> > > to 'DefaultAddrSpaceMap' from clang/lib/basic/TargetInfo.cpp
>>> > >
>>> > > It seems like the clang target that clover is using doesn't have
>>> > > an address space map initialized at all at this time... I'm not
>>> > > sure if that's because we're setting up the clang instance
>>> > > incorrectly (wrong triple), or if that map is just never
>>> > > initialized in the AMDGPU
>>> > > target(s) in LLVM. I've found the AMDGPUAS struct in AMDGPU.h which contains the address space mappings that are shared among subtargets and the getAMDGPUAS(Triple) function in AMDGPUBaseInfo.cpp, but I'm not sure where that information gets copied over to something that's returned by TargetInfo.getAddressSpaceMap(). Hopefully someone who knows that code better than I can tell me if I'm missing something.
>>> > >
>>> > > --Aaron
>>> > >
>>> > >
>>> > >
>>> > > > thanks for working on this.
>>> > > > Jan
>>> > > >
>>> > > > >
>>> > > > > However I am not sure if there is other changes need to be made.
>>> > > > >
>>> > > > > Thanks.
>>> > > > >
>>> > > > > Sam
>>> > > > >
>>> > > > > -----Original Message-----
>>> > > > > From: Jan Vesely [mailto:jv356 at scarletmail.rutgers.edu] On
>>> > > > > Behalf Of Jan Vesely
>>> > > > > Sent: Tuesday, June 27, 2017 12:17 PM
>>> > > > > To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Michel Dänzer
>>> > > > > <michel at daenzer.net>
>>> > > > > Cc: libclc-dev at lists.llvm.org
>>> > > > > Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>>> > > > >
>>> > > > > On Tue, 2017-06-27 at 14:42 +0000, Liu, Yaxun (Sam) wrote:
>>> > > > > > I am trying to build mesa with opencl enabled on Ubuntu
>>> > > > > > 16.04 so that I can reproduce the issue, however I got some issue.
>>> > > > > > After I build and install it, I cannot see the OpenCL icd
>>> > > > > > for mesa under /etc/OpenCL/vendors.
>>> > > > > >
>>> > > > > > Here is the command I use to build mesa:
>>> > > > > >
>>> > > > > > ./autogen.sh --prefix=/usr
>>> > > > > > --libdir=/usr/lib/x86_64-linux-gnu
>>> > > > > > --with- llvm-prefix=/usr/llvm/x86_64-linux-gnu
>>> > > > > > --enable-glx-tls
>>> > > > > > --enable- texture-float --disable-xvmc --disable-nine
>>> > > > > > --with-gallium- drivers=radeonsi,swrast --with-dri-drivers=
>>> > > > > > --with-egl- platforms=x11,drm --enable-gles1 --enable-gles2
>>> > > > > > -enable-opencl CXXFLAGS="-O2" CFLAGS="-O2"
>>> > > > > > make
>>> > > > > > sudo make install
>>> > > > > >
>>> > > > > > Can someone take a look if I missed something? Thanks.
>>> > > > >
>>> > > > > I think you need --enable-opencl_icd as well, otherwise mesa
>>> > > > > provides the root opencl library (LD_PRELOAD, or
>>> > > > > LD_LIBRARY_PATH should be enough to make it work even without
>>> > > > > icd)
>>> > > > >
>>> > > > > >
>>> > > > > > BTW my email was always bounced back from libclc-dev at lists.llvm.org.
>>> > > > > > I tried to subscribe to it for several times but never got response.
>>> > > > > > Can someone pass my email to the mailing list?
>>> > > > >
>>> > > > > no idea how to do that.
>>> > > > > AFAIK, Tom Stellard is the official maintainer of libclc. He might know better.
>>> > > > >
>>> > > > > regards,
>>> > > > > Jan
>>> > > > >
>>> > > > > >
>>> > > > > > Thanks.
>>> > > > > >
>>> > > > > > Sam
>>> > > > > >
>>> > > > > > -----Original Message-----
>>> > > > > > From: Liu, Yaxun (Sam)
>>> > > > > > Sent: Thursday, June 22, 2017 4:39 PM
>>> > > > > > To: 'Jan Vesely' <jan.vesely at rutgers.edu>; Michel Dänzer
>>> > > > > > <michel at daenzer.net>
>>> > > > > > Cc: libclc-dev at lists.llvm.org
>>> > > > > > Subject: RE: [Libclc-dev] clang SVN r303370 broke libclc
>>> > > > > >
>>> > > > > > Sorry for the delay. I am still working on it. I think the failure was due to some kernel argument metadata change.
>>> > > > > >
>>> > > > > > Sam
>>> > > > > >
>>> > > > > > -----Original Message-----
>>> > > > > > From: Jan Vesely [mailto:jv356 at scarletmail.rutgers.edu] On
>>> > > > > > Behalf Of Jan Vesely
>>> > > > > > Sent: Thursday, June 22, 2017 11:31 AM
>>> > > > > > To: Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; Michel Dänzer
>>> > > > > > <michel at daenzer.net>
>>> > > > > > Cc: libclc-dev at lists.llvm.org
>>> > > > > > Subject: Re: [Libclc-dev] clang SVN r303370 broke libclc
>>> > > > > >
>>> > > > > > Hi Sam,
>>> > > > > >
>>> > > > > > is there any progress on this regression? I still see the test failing on my machine.
>>> > > > > >
>>> > > > > > thanks,
>>> > > > > > Jan
>>> > > > > >
>>> > > > > > On Thu, 2017-05-25 at 10:18 -0400, Jan Vesely wrote:
>>> > > > > > > you can find the tests here:
>>> > > > > > > https://piglit.freedesktop.org/
>>> > > > > > >
>>> > > > > > > it's fairly straightforward to setup. my run cmdline is:
>>> > > > > > >
>>> > > > > > > OCL_ICD_VENDORS=/etc/OpenCL/vendors/mesa-git.icd python3
>>> > > > > > > ./piglit run tests/cl.py -o results/turks_cl_`date`
>>> > > > > > >
>>> > > > > > > the to generate html report I run:
>>> > > > > > >
>>> > > > > > > ./piglit summary html summary/turks_cl results/turks_cl_*
>>> > > > > > >
>>> > > > > > > it looks like this:
>>> > > > > > > http://paul.rutgers.edu/~jv356/piglit/radeon-latest-5/pro
>>> > > > > > > b
>>> > > > > > > lem
>>> > > > > > > s.ht
>>> > > > > > > ml
>>> > > > > > >
>>> > > > > > > Jan
>>> > > > > > >
>>> > > > > > > just fyi the 'set kernel argument for array' test is also
>>> > > > > > > local memory related.
>>> > > > > > >
>>> > > > > > > On Thu, 2017-05-25 at 14:10 +0000, Liu, Yaxun (Sam) wrote:
>>> > > > > > > > Is these tests part of libclc? How do I run them?
>>> > > > > > > >
>>> > > > > > > > Sam
>>> > > > > > > >
>>> > > > > > > > -----Original Message-----
>>> > > > > > > > From: Michel Dänzer [mailto:michel at daenzer.net]
>>> > > > > > > > Sent: Thursday, May 25, 2017 3:46 AM
>>> > > > > > > > To: Jan Vesely <jan.vesely at rutgers.edu>; Liu, Yaxun
>>> > > > > > > > (Sam) <Yaxun.Liu at amd.com>
>>> > > > > > > > Cc: libclc-dev at lists.llvm.org
>>> > > > > > > > Subject: Re: [Libclc-dev] clang SVN r303370 broke
>>> > > > > > > > libclc
>>> > > > > > > >
>>> > > > > > > > On 25/05/17 06:14 AM, Jan Vesely via Libclc-dev wrote:
>>> > > > > > > > > On Wed, 2017-05-24 at 16:22 -0400, Jan Vesely wrote:
>>> > > > > > > > > > On Wed, 2017-05-24 at 18:13 +0900, Michel Dänzer via Libclc-dev wrote:
>>> > > > > > > > > > > On 24/05/17 01:19 AM, Liu, Yaxun (Sam) wrote:
>>> > > > > > > > > > > > I have fixed the issue by
>>> > > > > > > > > > > > https://reviews.llvm.org/rL303644
>>> > > > > > > > > > >
>>> > > > > > > > > > > Seems to work fine, thanks!
>>> > > > > > > > > >
>>> > > > > > > > > > also seems to fix most of EG 670 opencl regressions.
>>> > > > > > > > >
>>> > > > > > > > > while at the same time it introduces regression in
>>> > > > > > > > > "set kernel argument for array" subtest on Turks.
>>> > > > > > > >
>>> > > > > > > > Actually, I'm also seeing that test and a bunch of
>>> > > > > > > > local atomic tests fail. Reverting both of Sam's
>>> > > > > > > > patches and rebuilding libclc and Mesa fixes them. Not
>>> > > > > > > > sure why I wasn't seeing these failures before, but it looks like there's still an issue here.
>>> > > > > > > >
>>> > > > > > > >
>>> > > > > > > > --
>>> > > > > > > > Earthling Michel Dänzer | http://www.amd.com
>>> > > > > > > > Libre software enthusiast | Mesa and X developer
>>> > > > > > > >
>>> > > > >
>>> > > > > --
>>> > > > > Jan Vesely <jan.vesely at rutgers.edu>
>>> > > >
>>> > > > _______________________________________________
>>> > > > Libclc-dev mailing list
>>> > > > Libclc-dev at lists.llvm.org
>>> > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/libclc-dev
>>> > > >
>>
>> --
>> Jan Vesely <jan.vesely at rutgers.edu>
More information about the Libclc-dev
mailing list