Three more patches

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 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 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?

– Johnny

lrb2mic_make_build.patch (44 KB)

lrb2mic_cmake_build.patch (22.5 KB)

ittnotify_fixes.patch (5.76 KB)

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

The lrb2mic_make_build.patch is definitely good to go. We’ve been testing it for a couple months now in our nightly builds with no problems.

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

I don’t want to block your patch, but I would love to see any and all functionality be testable by non-intel compilers if and where possible.

Can you give the dummies step by step instructions on how to test/validate this feature. Maybe starting a document called TESTING or similar is a good idea. If the Intel weekly results aren’t publicly available then that’s not useful to anyone outside of Intel. I won’t know if I broke something when I change the code…

Thanks

I’ve committed the third patch, revision 226283.

We will think of your request to document interface(s) between tool(s) and the library. Ideally we would be willing to move to standard interface (e.g. OMPT which is supposed to be included in future version of the OpenMP specification), and probably get rid of Intel-specific interface then. I fully agree with your comment that all the changes should be validated as much as possible, even such that ittnotify code that does not play at all without the presence of specific performance tool.

Thanks,

Andrey