At 2013-09-03 00:12:56,"Tobias Grosser" <tobias at grosser.es> wrote:>On 09/02/2013 07:44 AM, Star Tan wrote: >> At 2013-09-02 16:22:28,"Tobias Grosser" <tobias at grosser.es> wrote: >> >>> On 09/01/2013 08:02 PM, Star Tan wrote: >>>> Hi all, >>>> >>>> >>>> Attached patch file to update lit config for Cloog. Without it, Polly always skips Cloog testings when we run "make check-polly". >>> >>> Dear Star Tan, >>> >>> thanks a lot for the patch. It looks very reasonable, but I am wondering >>> why it was not needed before or what problem it fixes exactly. Could you >>> add some information about this to the commit message. >>> >> I am not sure why it was not needed before; maybe it has never worked well before. The problem is that Polly never executes Cloog specific testcases no matther whether Cloog is found or not. I find this problem because I put a new testcase in test/Cloog/CodeGen/, but it is never executed. >> @Sebastian, you added the Cloog directory in r169159, including all testcases and the lit.local.cfg. The lit.local.cfg ensures that cloog specific testcases are executed only with CLOOG_FOUND by adding the following lit commands in test/Cloog/lit.local.cfg: >> cloog = config.root.cloog_found >> if cloog not in ['TRUE', 'true'] : >> config.unsupported = True >> However, there are two problems: >> First, since the cloog_found is set as "@CLOOG_FOUND@", I think the following "sed" command should be added into Makefile: >> sed -e "s#@CLOOG_FOUN@#$(CLOOG_FOUND)#g" >> Unfortunately such command is missed in current Polly. If it is not needed, I am curious how could Polly determine the value of @CLOOG_FOUND@ ? >> Second, even if we add the "sed" command in Makefile, the config.root.cloog_found would be set as "yes" or null, but Polly currently compares it with "TRUE" or "true", which thus always fails and the Cloog testcases will never be executed. >> @Sebastian, could you do me a favor to have a review of this problem? FYI, I have re-attached the patch file. > >This is surprising. Even without your patch my test/lit.site.cfg file >contains: > >config.enable_gpgpu_codegen = "" >config.cloog_found = "TRUE" > >(With CLOOG enabled and GPGPU_codegen disabled). Also, the CLooG test >are run (and are failing) as expected. > >I think it would be good to understand why this different behaviour can >be observed. Sebastian, any ideas? >Interesting! In my computer, it shows as: config.enable_gpgpu_codegen = "@CUDALIB_FOUND@" config.cloog_found = "@CLOOG_FOUND@" The source version is: LLVM: r189730; Polly: r189177; Clang:r189731 Best, Star Tan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130903/5282918c/attachment.html>
Hi, Star Tan wrote:> At 2013-09-03 00:12:56,"Tobias Grosser" <tobias at grosser.es> wrote: > > >On 09/02/2013 07:44 AM, Star Tan wrote: > >> At 2013-09-02 16:22:28,"Tobias Grosser" <tobias at grosser.es> wrote: > >> > >>> On 09/01/2013 08:02 PM, Star Tan wrote: > >>>> Hi all, > >>>> > >>>> > >>>> Attached patch file to update lit config for Cloog. Without it, Polly always skips Cloog testings when we run "make check-polly". > >>> > >>> Dear Star Tan, > >>> > >>> thanks a lot for the patch. It looks very reasonable, but I am wondering > >>> why it was not needed before or what problem it fixes exactly. Could you > >>> add some information about this to the commit message. > >>> > >> I am not sure why it was not needed before; maybe it has never worked well before. The problem is that Polly never executes Cloog specific testcases no matther whether Cloog is found or not. I find this problem because I put a new testcase in test/Cloog/CodeGen/, but it is never executed. > >> @Sebastian, you added the Cloog directory in r169159, including all testcases and the lit.local.cfg. The lit.local.cfg ensures that cloog specific testcases are executed only with CLOOG_FOUND by adding the following lit commands in test/Cloog/lit.local.cfg: > >> cloog = config.root.cloog_found > >> if cloog not in ['TRUE', 'true'] : > >> config.unsupported = True > >> However, there are two problems: > >> First, since the cloog_found is set as "@CLOOG_FOUND@", I think the following "sed" command should be added into Makefile: > >> sed -e "s#@CLOOG_FOUN@#$(CLOOG_FOUND)#g" > >> Unfortunately such command is missed in current Polly. If it is not needed, I am curious how could Polly determine the value of @CLOOG_FOUND@ ? > >> Second, even if we add the "sed" command in Makefile, the config.root.cloog_found would be set as "yes" or null, but Polly currently compares it with "TRUE" or "true", which thus always fails and the Cloog testcases will never be executed. > >> @Sebastian, could you do me a favor to have a review of this problem? FYI, I have re-attached the patch file. > > > >This is surprising. Even without your patch my test/lit.site.cfg file > >contains: > > > >config.enable_gpgpu_codegen = "" > >config.cloog_found = "TRUE" > > > >(With CLOOG enabled and GPGPU_codegen disabled). Also, the CLooG test > >are run (and are failing) as expected. > > > >I think it would be good to understand why this different behaviour can > >be observed. Sebastian, any ideas? > > > Interesting! In my computer, it shows as: > > config.enable_gpgpu_codegen = "@CUDALIB_FOUND@" > config.cloog_found = "@CLOOG_FOUND@"I think the problem is: lit.site.cfg.in:config.cloog_found = "@CLOOG_FOUND@" it should be lower case letters as in: Makefile.config.in:CLOOG_FOUND := @cloog_found@ because when we detect cloog, we use the function in find_lib_and_headers.m4 find_lib_and_headers([cloog], [cloog/isl/cloog.h], [cloog-isl]) ./autoconf/m4/find_lib_and_headers.m4: AC_SUBST([$1_found],["yes"]) and this will be instantiated as AC_SUBST([cloog_found],["yes"]). A patch that changes lit.site.cfg.in - config.enable_gpgpu_codegen = "@CUDALIB_FOUND@" - config.cloog_found = "@CLOOG_FOUND@" + config.enable_gpgpu_codegen = "@cudalib_found@" + config.cloog_found = "@cloog_found@" is preaproved. Thanks, Sebastian -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Sebastian, thanks for your reply! At 2013-09-27 03:33:04,"Sebastian Pop" <spop at codeaurora.org> wrote:>Hi,> >Star Tan wrote: >> At 2013-09-03 00:12:56,"Tobias Grosser" <tobias at grosser.es> wrote: >> >> >On 09/02/2013 07:44 AM, Star Tan wrote: >> >> At 2013-09-02 16:22:28,"Tobias Grosser" <tobias at grosser.es> wrote: >> >> >> >>> On 09/01/2013 08:02 PM, Star Tan wrote: >> >>>> Hi all, >> >>>> >> >>>> >> >>>> Attached patch file to update lit config for Cloog. Without it, Polly always skips Cloog testings when we run "make check-polly". >> >>> >> >>> Dear Star Tan, >> >>> >> >>> thanks a lot for the patch. It looks very reasonable, but I am wondering >> >>> why it was not needed before or what problem it fixes exactly. Could you >> >>> add some information about this to the commit message. >> >>> >> >> I am not sure why it was not needed before; maybe it has never worked well before. The problem is that Polly never executes Cloog specific testcases no matther whether Cloog is found or not. I find this problem because I put a new testcase in test/Cloog/CodeGen/, but it is never executed. >> >> @Sebastian, you added the Cloog directory in r169159, including all testcases and the lit.local.cfg. The lit.local.cfg ensures that cloog specific testcases are executed only with CLOOG_FOUND by adding the following lit commands in test/Cloog/lit.local.cfg: >> >> cloog = config.root.cloog_found >> >> if cloog not in ['TRUE', 'true'] : >> >> config.unsupported = True >> >> However, there are two problems: >> >> First, since the cloog_found is set as "@CLOOG_FOUND@", I think the following "sed" command should be added into Makefile: >> >> sed -e "s#@CLOOG_FOUN@#$(CLOOG_FOUND)#g" >> >> Unfortunately such command is missed in current Polly. If it is not needed, I am curious how could Polly determine the value of @CLOOG_FOUND@ ? >> >> Second, even if we add the "sed" command in Makefile, the config.root.cloog_found would be set as "yes" or null, but Polly currently compares it with "TRUE" or "true", which thus always fails and the Cloog testcases will never be executed. >> >> @Sebastian, could you do me a favor to have a review of this problem? FYI, I have re-attached the patch file. >> > >> >This is surprising. Even without your patch my test/lit.site.cfg file >> >contains: >> > >> >config.enable_gpgpu_codegen = "" >> >config.cloog_found = "TRUE" >> > >> >(With CLOOG enabled and GPGPU_codegen disabled). Also, the CLooG test >> >are run (and are failing) as expected. >> > >> >I think it would be good to understand why this different behaviour can >> >be observed. Sebastian, any ideas? >> > >> Interesting! In my computer, it shows as: >> >> config.enable_gpgpu_codegen = "@CUDALIB_FOUND@" >> config.cloog_found = "@CLOOG_FOUND@" > >I think the problem is: > >lit.site.cfg.in:config.cloog_found = "@CLOOG_FOUND@" > >it should be lower case letters as in: > >Makefile.config.in:CLOOG_FOUND := @cloog_found@ > >because when we detect cloog, we use the function in find_lib_and_headers.m4 > >find_lib_and_headers([cloog], [cloog/isl/cloog.h], [cloog-isl]) > >./autoconf/m4/find_lib_and_headers.m4: AC_SUBST([$1_found],["yes"]) > >and this will be instantiated as AC_SUBST([cloog_found],["yes"]). > >A patch that changes lit.site.cfg.in > >- config.enable_gpgpu_codegen = "@CUDALIB_FOUND@" >- config.cloog_found = "@CLOOG_FOUND@" >+ config.enable_gpgpu_codegen = "@cudalib_found@" >+ config.cloog_found = "@cloog_found@" >If you change "CLOOG_FOUND" to "cloog_found", which means the final lit.site.cfg will looks like: config.cloog_found = yes However, the lit.local.cfg in Cloog directory is: cloog = config.root.cloog_found if cloog not in ['TRUE', 'true'] : config.unsupported = True which means it will compare the config_found with 'TRUE'/'true', not 'yes'. So, it still does not work. That's why I propose to modify the lit.local.cfg to compare cloog_found with 'yes' rather than 'TRUE' or 'true'. Did I miss something? Star Tan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130927/a701df1c/attachment.html>