Data Parallel C++ implementation of c_sw kernel. by jtullos-intel · Pull Request #47 · NOAA-GSL/SENA-c_sw

jtullos-intel · 2021-11-19T14:20:45Z

Co-authored-by: Christoph Bauinger c.bauinger@gmx.at

Proposed changes

This is a Data Parallel C++ implementation of the c_sw kernel. The intent is for evaluation of new language options for potential migration.

Co-authored-by: Christoph Bauinger <c.bauinger@gmx.at>

robgpita · 2021-11-22T21:25:27Z

@@ -0,0 +1,79 @@
+This is a Data Parallel C++ implementation of the `c_sw` kernel extracted from the FV3 model.


I believe we should include a header:

The Data Parallel C++ C_SW Kernel.

as well as a Dependencies & Prerequisites sections to keep these three implementations standard. This could be a separate PR, but want to mention it.

robgpita

This is a huge addition. Thanks for the work in implementing the DPC++ version of c_sw!
I do not know C++ well, so I can not be too detailed in my review of the actual DPC++ syntax. I believe the compilation errors should be addressed as well as some other comments I've made before any of this code is merged. Not sure if others are having issues in compiling and testing this code, but I'm assuming the only dependency is the IntelOne API?

robgpita · 2021-11-22T21:34:52Z

+  add_definitions( -DNDEBUG )
+endif( )
+
+# Set flags depending on which compiler iis used


Typo, iis. Same in line 12 & 19.

robgpita · 2021-11-22T21:52:51Z

+  }
+#endif
+
+}


No newline here.

robgpita · 2021-11-22T21:53:03Z

+        Offset2DArray<double, true>& ua, Offset2DArray<double, true>& va, 
+        Offset2DArray<double, true>& divg_d,
+        Offset2DArray<double, true>& uf, Offset2DArray<double, true>& vf) const;
+};


No newline.

robgpita · 2021-11-22T21:57:43Z

+    {
+        Deallocate(in.size(), in.values, out, Q);
+    }
+};


No newline.

robgpita · 2021-11-22T22:02:02Z

+)
+
+SET(OBJS
+  /global/panfs01/admin/opt/intel/oneAPI/2021.1.2/compiler/2021.1.2/linux/compiler/lib/intel64_lin/for_main.o


Should this directory be hardcoded?

During the configure process CMake detects it cannot find this file and errors. Why do we include this object file (that comes with the compiler) with the rest of the compiled code? If required, then we'll need to detect the location from the compiler installation. I also have this file inside the 2021.3.0 release but obviously at a different path.

If I comment it out then the CMake configure completes. Although, it may be needed when linking and I cannot get that far in the build process.

robgpita · 2021-11-23T04:55:27Z

@@ -0,0 +1,95 @@
+! Copyright (C) 2020-2021 Intel Corporation
+


It doesn't appear as though there is anything aside from a blank line in this PR, but I'm getting two errors ( Error: Invalid character in name at (1) & Error: Invalid character 0xC2 at (1) ) in lines 2, 6, 8, and 14 when trying to compile.

My build crashes with errors on line 2, ,6, 8, 14. Line lines at those locations start with the "_" character.

git/SENA-c_sw/dpcpp/src/new_sw_core_interfaces.f90(2): error #5078: Unrecognized token '?' skipped

robgpita · 2021-11-23T04:56:40Z

+          real,    intent(out), dimension(*) :: divg_d
+        end subroutine divergence_corner_from_f
+      end interface
+      end module


No newline.

robgpita · 2021-11-23T05:22:59Z

+  jep1 = je + 1;
+
+  d2a2c_vect(sw_corner, se_corner, ne_corner, nw_corner, sin_sg, cosa_u,
+      cosa_v, cosa_s, rsin_u, rsin_v, rsin2, dxa, dya, u, v, ua, va, uc, vc,


I'm getting compilation errors stemming from this line.

/home/ubuntu/dpcpp/SENA-c_sw/dpcpp/src/new_sw_core.cpp:511:58: error: use of undeclared identifier 'sin_sg' d2a2c_vect(sw_corner, se_corner, ne_corner, nw_corner, sin_sg, cosa_u,

fatal error: too many errors emitted, stopping now [-ferror-limit=] 20 errors generated

robgpita · 2021-11-23T05:23:18Z

+    memallocator.Deallocate<double, true>(dxa, nullptr, Q);
+    memallocator.Deallocate<double, true>(dya, nullptr, Q);
+
+}


No newline.

robgpita · 2021-11-23T16:27:21Z

+                    d.utmp(i, j,k) = d.a2 * (d.u(i, j-1,k) + d.u(i, j+2,k)) + d.a1 * (d.u(i, j,k) + d.u(i, j+1,k));
+            //    }
+            //}
+        });


Out of curiosity, is the .wait() omitted here because this loop runs very quickly?

Good question... Initially I thought it was because nothing depended on this value in the following code. Except, the vtmp variable below has a wait. They both cannot be correct. Both utmp and vtmp are used at line 480 so my guess this should have a wait.

BryanFlynt · 2021-11-30T05:15:39Z

+#
+# The following paths will be searched in order if set in CMake (first priority) or environment (second priority)
+#
+#   - NetCDF_ROOT                 - root of NetCDF installation


This is probably going to be an issue in the future. NetCDF has split C, C++, Fortran into separate projects that must be compiled and installed separately. The version numbers have diverging rapidly so a single NetCDF_ROOT variable is going to need to be more specific for future releases. Something along the lines of NetCDF_C_ROOT, NetCDF_CXX_ROOT and NetCDF_F_ROOT is going to be needed.

BryanFlynt · 2021-11-30T06:08:21Z

+#include <chrono>
+
+#include "new_sw_core.hpp"
+#include <CL/sycl.hpp>


I get compiler errors nested pretty deep within the OneAPI provided CL/sycl.hpp include.

In file included from apps/oneapi/2021.3.0/compiler/2021.3.0/linux/include/sycl/CL/sycl/ONEAPI/atomic_enums.hpp(12), from apps/oneapi/2021.3.0/compiler/2021.3.0/linux/include/sycl/CL/sycl/ONEAPI/atomic_accessor.hpp(11), from /apps/oneapi/2021.3.0/compiler/2021.3.0/linux/include/sycl/CL/sycl/ONEAPI/atomic.hpp(11), from apps/oneapi/2021.3.0/compiler/2021.3.0/linux/include/sycl/CL/sycl.hpp(11), from git/SENA-c_sw/dpcpp/src/new_sw_core.hpp(24), from git/SENA-c_sw/dpcpp/src/new_sw_core.cpp(25): apps/oneapi/2021.3.0/compiler/2021.3.0/linux/include/sycl/CL/sycl/detail/common.hpp(318): error: no default constructor exists for class "cl::sycl::mode_tag_t<(cl::sycl::access_mode={cl::sycl::access::mode})1024>" static constexpr T value{};

BryanFlynt

This still needs some work before it's portable outside of Intel's development environment. We have a hard coded path in the cmake file and some phantom "_" characters in the F code. When I work around these I still get compiler errors inside the OneAPI library itself. My guess is those are a result of compiler versions or system configuration so we need to work out a way to make this work for the average user.

Data Parallel C++ implementation of c_sw kernel.

2340744

Co-authored-by: Christoph Bauinger <c.bauinger@gmx.at>

christopherwharrop-noaa requested review from BryanFlynt-NOAA, christopherwharrop-noaa and robgpita November 19, 2021 15:27

robgpita reviewed Nov 22, 2021

View reviewed changes

robgpita reviewed Nov 23, 2021

View reviewed changes

BryanFlynt reviewed Nov 30, 2021

View reviewed changes

BryanFlynt suggested changes Nov 30, 2021

View reviewed changes

		@@ -0,0 +1,79 @@
		This is a Data Parallel C++ implementation of the `c_sw` kernel extracted from the FV3 model.

+                }
+              #endif
+              }

                
                    No newline at end of file

Uh oh!

Conversation

jtullos-intel commented Nov 19, 2021

Proposed changes

Uh oh!

robgpita Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

The Data Parallel C++ C_SW Kernel.

Uh oh!

robgpita left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanFlynt Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanFlynt Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanFlynt Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanFlynt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robgpita Nov 22, 2021 •

edited

Loading

BryanFlynt Nov 30, 2021 •

edited

Loading

BryanFlynt Nov 30, 2021 •

edited

Loading

BryanFlynt Nov 30, 2021 •

edited

Loading