[PATCH] D36582: [test-suite] Adding HPCCG benchmark

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Hi Brian, Hal,

In https://reviews.llvm.org/D36582#839270, @hfinkel wrote:

> In https://reviews.llvm.org/D36582#838188, @kristof.beyls wrote:
>
> > Hi @homerdin,
> >
> > Great to see you taking the effort to make the test-suite more relevant!
> >  I think it'd be helpful if you could also add a little explanation of how adding this code makes the test-suite more relevant.
> >  I'm assuming that this kind of code is common in some domain (HPC?) and that there is no other benchmark already in the test-suite with similar enough properties?
>
>
> Just to provide some context: In https://reviews.llvm.org/D27311, I added one of our HPC benchmarks (XSBench), and this is a second one. Brian and several other students working for me this summer have gone through all 40 of the possible additions on our current list (https://gitlab.com/llvm-doe/public/wikis/DOEProxyApps) and narrowed the list down to approximately 15 that are appropriate for inclusion into the test suite. They're all now working on preparing patches for those.  This will increase our coverage of relevant HPC algorithms and code patterns.
>
> HPCCG represents a conjugate gradients calculation with a 27-point stencil. It's probably most similar to Benchmarks/ASCI_Purple/SMG2000, but HPCCG's implementation looks different (and is in C++, not C, and is a decade+ newer). FWIW, given the relatively-small size of our test suite, I'm not too concerned about similarity (so long as the codes actually are different).
>
> Thanks!


Great, thanks for the context, that makes a lot of sense!

In https://reviews.llvm.org/D36582#839398, @homerdin wrote:

> Thanks for the feedback.  I have updated the LICENSE.TXT file to include this application.  The machine I used was an IBM x3550 M4.
>
> Let me know if there anything else you would like to know,
> Brian


No thanks, that's enough. I was just curious about roughly what kind of machine you did your timing measurements on.

Overall, this looks good to me, I've just got 2 more minor remarks after looking in more detail at the code.

I'm also assuming that you've attempted to keep the code close to the upstream version at https://gitlab.com/llvm-doe/public/wikis/DOEProxyApps, as it seems there's quite a bit of dead code in here for how this code will be used in the test-suite (YAML, mytimer, dump_matlab_matrix, ...). If my assumption about staying close to the upstream version: that makes sense to me.
Please note that I'm not qualified to judge the actual kernel implemented here, but I'm assuming that has been reviewed before enough by qualified people.

Thanks!

Kristof



================
Comment at: MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/Makefile:4
+PROG = HPCCG
+CPPFLAGS = -DVERIFICATION
+LDFLAGS = -lm
----------------
I can't seem to find -DVERIFICATION as a CPPFLAGS in the CMakelists.txt, which seems inconsistent.
Should -DVERIFICATION be added to both Makefile and CMakelists.txt or neither?


================
Comment at: MultiSource/Benchmarks/DOE-ProxyApps-C++/HPCCG/README:57-62
+By default the code compiles with MPI support and can be run on one 
+or more processors.  If you don't have MPI, or want to compile without 
+MPI support, you may change the definition of USE_MPI in the 
+makefile, or use make as follows:
+
+make USE_MPI=
----------------
I would hope the default will be to run without MPI support enabled?
Is this part of the README wrong, or does USE_MPI= have to be enabled somewhere?
Or I guess that the makefile referred to is the Makefile at https://gitlab.com/llvm-doe/public/wikis/DOEProxyApps, instead of the custom ones here?
Maybe it'd be best overall to start this README by explaining that the original source is at https://gitlab.com/llvm-doe/public/wikis/DOEProxyApps; and that the rest of the README describes that original source? And maybe with the note that the Makefile/CMakeLists.txt is the main difference from the originals (if that really is the case)?
I think leaving the README here is very valuable, there some great explanations in here in case someone needs to explore this program further.


https://reviews.llvm.org/D36582