[Openmp-commits] [Openmp-dev] Three more patches

[Churbanov, Andrey]

OK, I have committed the lrb2mic* patches (revisions 226271, 226272).

As to ittnotify fixes/testing, we don't have such tests.  We rely on a regular (weekly) testing of ittnotify functionality in our library by the Intel(R) VTune Amplifier quality assurance team.  Theoretically it is possible to write a test that would check this functionality without running VTune, but this would require the test to emulate a lot of functionality of the tool: provide ittnotify library (e.g. built from sources at https://software.intel.com/en-us/articles/intel-itt-api-open-source), register this library within the OpenMP RTL, collect data reported by the OpenMP RTL, analyze the data, report the result.  We just don't have the will and resources to write such a complicated test, given that the functionality is tested regularly by our colleagues from VTune team.

Can I commit the ittnotify patch without providing the test?

Thanks,
Andrey

From: openmp-dev-bounces at cs.uiuc.edu [mailto:openmp-dev-bounces at cs.uiuc.edu] On Behalf Of C Bergstrom
Sent: Thursday, January 15, 2015 9:45 AM
To: Peyton, Jonathan L
Cc: openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: Re: [Openmp-dev] Three more patches


On Thu, Jan 15, 2015 at 6:59 AM, Peyton, Jonathan L <jonathan.l.peyton at intel.com<mailto:jonathan.l.peyton at intel.com>> wrote:
I have three more patches here and there descriptions below.  Lrb2mic_cmake_build.patch depends on lrb2mic_make_build.patch so it must go after. But the ittnotify_fixes.patch is independent of the rest.

Lrb2mic_make_build.patch – This patch (and the similarly named lrb2mic_cmake_build.patch) gets rid of all the references to lrb when building for Intel® Many Integrated Core Architecture.  I know most people aren’t compiling for this architecture and this patch is somewhat invasive, but it does not affect the Intel® 64 or IA-32 builds, nor any other architecture’s build.  Here are the main features this patch fixes:
1) exports directory renamed to lin_knc, depending on which MIC flavor you are compiling for,
2) The internal perl scripts and library modules now have a variable $target_mic_arch which holds the value of “knc”, “knf”, (“knl” in the future) for Knights Corner, Knights Ferry, and Knights Landing.
3) tools used for checks are properly named to use MIC specific tools (e.g., instead of objcopy use x86_64-k1om-linux-objcopy)
4) build.pl<http://build.pl> doesn’t take os=lrb, but instead takes arch=mic & mic_arch=knc.  This is done under the hood of the top level Makefile.  You still build using the top level Makefile by typing $ make compiler=icc arch=mic
5) During this change, I got rid of some extraneous build.pl<http://build.pl> options (e.g., tcheck, target-compiler, etc.) which aren’t used anymore.

Lrb2mic_cmake_build.patch – This patch is the CMake changes that correspond to the above patch.  This patch is built off the last one and is only the CMake files that are affected.  This patch also does a few other things:
1) I changed some compiler flag settings for icc only that were wrong for MIC builds.
2) Instead of explicit linking to –ldl , I changed cmake to use CMAKE_DL_LIBS which links the –ldl library if it exists on platform you are compiling for.
3) Now to compile for MIC you specify –Dos=lin –Darch=mic instead of –Dos=mic ; it is an architecture after all that runs Linux as the os.

Ittnotify_fixes.patch – This patch fixes a few things concerning the ittnotify interface.
1) Metadata for single construct used to be reported from all threads.  This was fixed to be reported only by the master.
2) Region-begin was mistakenly saved for any active level but had to be saved only for the first level.
3) Added code to report location for #omp single metadata.

To apply patches:
$ patch –p0 < lrb2mic_make_build.patch
$ patch –p0 < lrb2mic_cmake_build.patch
$ patch –p0 < ittnotify_fixes.patch

Comments or Questions?

For the ittnotify fixes is there any corresponding test case? Even if we don't have a test harness which can easily run it, having something would be great so we can get in the habit of requiring this for bug fixes.

For the other patches maybe someone @Intel can test and sign off on the cod review

Thanks

--------------------------------------------------------------------
Closed Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park, 
17 Krylatskaya Str., Bldg 4, Moscow 121614, 
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/openmp-commits/attachments/20150116/98c4dee2/attachment.html>