Fixing #321 issue #341

ipdemes · 2022-05-10T03:49:48Z

Adding logic for dispatch for cunumeric-specific types
Adding logic for the copy operation on cunumeric types (PointN)
Adding logic in advanced indexing for copies with the indirect field of shape (1,)

This PR takes another approach for fixing Issue #321

- Adding logic for dispatch for cunumeric-specific types - Adding logic for the copy operation on cunumeric types (PointN) - Adding logic in advanced indexing for copies with the indirect field of shape (1,)

magnatelee · 2022-05-10T20:22:33Z

cunumeric/deferred.py

+    def _convert_future_to_store(self, a):
+        store = self.context.create_store(
+            a.dtype,
+            shape=(1,),


Is there a reason you don't pass a.shape here? I suspect doing this could potentially require broadcasting later if the shape was not (1,) but some other degenerate case (e.g., (1,1,)).

I have assumed that shape for the Future is always (1,). I will change it to a.shape

magnatelee · 2022-05-10T20:23:29Z

cunumeric/deferred.py

+                    index_array = self._convert_future_to_store(index_array)
+                    result_store = self.context.create_store(
+                        self.dtype,
+                        shape=(1,),


same question here: why can't we just use index_array.shape?

magnatelee · 2022-05-10T20:26:10Z

src/cunumeric/utilities/dispatch.h

+    dim1, dim2, std::forward<Functor>(f), std::forward<Fnargs>(args)...);
+}
+
+// template <typename Functor, typename... Fnargs>


Remove these comments.

magnatelee · 2022-05-10T20:40:59Z

src/cunumeric/utilities/dispatch.h

+        return f.template operator()<CuNumericTypeCodes::CUNUMERIC_TYPE_POINT4, DIM>(
+          std::forward<Fnargs>(args)...);
+      }
+      case CuNumericTypeCodes::CUNUMERIC_TYPE_POINT5: {


I think this dispatch should abide by LEGION_MAX_DIM and avoid instantiating types that will never arise for a given LEGION_MAX_DIM. I guess you could check code <= MAX_TYPE_NUMBER early on so that you can fall back to the core's dispatch more quickly.

magnatelee · 2022-05-10T20:41:42Z

src/cunumeric/utilities/dispatch.h

+#endif
+#if LEGION_MAX_DIM >= 2
+    case 2: {
+      return inner_type_dispatch_fn<2>{}(code, f, std::forward<Fnargs>(args)...);


why is this not cunumeric::inner_type_dispatch_fn? (same question to other cases)

Yes, it should have cunumeric:: in front.

magnatelee · 2022-05-10T20:42:25Z

src/cunumeric/utilities/dispatch.h

+}
+
+template <typename Functor, typename... Fnargs>
+constexpr decltype(auto) double_dispatch(int dim1, int dim2, Functor f, Fnargs&&... args)


where are you using this?

I am not using this, but might in the future. Would you recommend removing currently unused functions?

what use case do you have in mind?

magnatelee · 2022-05-10T20:43:11Z

src/cunumeric/utilities/dispatch.h

+// }
+
+template <typename TypeCode, typename Functor, typename... Fnargs>
+constexpr decltype(auto) type_dispatch(TypeCode code, Functor f, Fnargs&&... args)


is this really used in the code? if so, this has the same issue as the inner dispatch of not abiding by LEGION_MAX_DIM.

magnatelee · 2022-05-10T20:46:08Z

src/cunumeric/unary/unary_op_template.inl

@@ -64,6 +69,32 @@ struct UnaryOpImpl {
  {
    assert(false);
  }
+
+  template <CuNumericTypeCodes CODE, int DIM>
+  void operator()(UnaryOpArgs& args) const


Can we move this to a separate functor instead of adding it to the existing one? I want use to avoid any accidental instantiations of this template.

Sounds good

magnatelee · 2022-05-10T20:47:47Z

src/cunumeric/unary/unary_op_template.inl

@@ -72,7 +103,7 @@ struct UnaryOpDispatch {
  void operator()(UnaryOpArgs& args) const
  {
    auto dim = std::max(args.in.dim(), 1);
-    double_dispatch(dim, args.in.code(), UnaryOpImpl<KIND, OP_CODE>{}, args);
+    cunumeric::double_dispatch(dim, args.in.code(), UnaryOpImpl<KIND, OP_CODE>{}, args);


I think you can do better than this by checking the op code from the outside and choosing a point copy functor if the op code was COPY. Then, you don't even need to do this custom double dispatch for all the other ops.

magnatelee · 2022-05-10T20:48:35Z

src/cunumeric/utilities/dispatch.h

+ *
+ */
+
+#pragma once


If this is used only in the unary op, I rather have it there than here. This hasn't become a general utility yet.

magnatelee · 2022-05-10T20:49:12Z

src/cunumeric/utilities/type_traits.h

+
+namespace cunumeric {
+
+template <CuNumericTypeCodes CODE>


same here. if you don't use this anywhere other than the unary op, keep this there.

cunumeric/deferred.py

src/cunumeric/unary/unary_op.h

manopapad · 2022-05-11T17:52:13Z

@ipdemes I have added some comments above. I suggest creating a new PR to address them.

manopapad · 2022-05-12T02:36:14Z

@ipdemes One more thing to note: Clang is throwing some warnings such as:

./cunumeric/unary/unary_op.h:80:12: warning: comparison of different enumeration types in switch statement ('legate_core_type_code_t' and 'CuNumericTypeCodes') [-Wenum-compare-switch]
      case CuNumericTypeCodes::CUNUMERIC_TYPE_POINT4: {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./cunumeric/unary/unary_op.h:131:14: note: in instantiation of function template specialization 'cunumeric::inner_type_dispatch_fn<1>::operator()<legate_core_type_code_t, cunumeric::UnaryCopyImpl<cunumeric::VariantKind::CPU>, cunumeric::UnaryOpArgs &>'
      requested here
      return cunumeric::inner_type_dispatch_fn<1>{}(code, f, std::forward<Fnargs>(args)...);
             ^
./cunumeric/unary/unary_op_template.inl:169:18: note: in instantiation of function template specialization 'cunumeric::double_dispatch<legate_core_type_code_t, cunumeric::UnaryCopyImpl<cunumeric::VariantKind::CPU>, cunumeric::UnaryOpArgs &>' requested
      here
      cunumeric::double_dispatch(dim, args.in.code(), UnaryCopyImpl<KIND>{}, args);
                 ^
./cunumeric/unary/unary_op_util.h:84:25: note: in instantiation of function template specialization 'cunumeric::UnaryOpDispatch<cunumeric::VariantKind::CPU>::operator()<cunumeric::UnaryOpCode::ABSOLUTE>' requested here
      return f.template operator()<UnaryOpCode::ABSOLUTE>(std::forward<Fnargs>(args)...);
                        ^
cunumeric/unary/unary_op.cc:106:3: note: in instantiation of function template specialization 'cunumeric::unary_op_template<cunumeric::VariantKind::CPU>' requested here
  unary_op_template<VariantKind::CPU>(context);
  ^

and

./cunumeric/unary/unary_op.h:68:12: warning: case value not in enumerated type 'legate_core_type_code_t' [-Wswitch]
      case CuNumericTypeCodes::CUNUMERIC_TYPE_POINT2: {
           ^

ipdemes · 2022-05-12T16:24:10Z

Thank you! I will try to fix it today

ipdemes requested a review from magnatelee May 10, 2022 03:49

Fixing nv-legate#321 issue

8760958

- Adding logic for dispatch for cunumeric-specific types - Adding logic for the copy operation on cunumeric types (PointN) - Adding logic in advanced indexing for copies with the indirect field of shape (1,)

ipdemes force-pushed the cunumeric_321 branch from fe57618 to 8760958 Compare May 10, 2022 04:03

magnatelee reviewed May 10, 2022

View reviewed changes

moving copy operation to a separate functor + code clean-up

8b30998

magnatelee self-requested a review May 11, 2022 03:27

magnatelee approved these changes May 11, 2022

View reviewed changes

ipdemes added 2 commits May 10, 2022 22:00

Merge remote-tracking branch 'origin/branch-22.05' into cunumeric_321

f2f79f4

Merge remote-tracking branch 'origin/branch-22.05' into cunumeric_321

b2655d2

ipdemes merged commit 26a9951 into nv-legate:branch-22.05 May 11, 2022

manopapad reviewed May 11, 2022

View reviewed changes

cunumeric/deferred.py Show resolved Hide resolved

manopapad reviewed May 11, 2022

View reviewed changes

src/cunumeric/unary/unary_op.h Show resolved Hide resolved

manopapad reviewed May 11, 2022

View reviewed changes

src/cunumeric/unary/unary_op.h Show resolved Hide resolved

magnatelee mentioned this pull request May 12, 2022

fixing compile-time warnings #351

Merged

ipdemes deleted the cunumeric_321 branch May 26, 2022 20:20

manopapad added a commit to manopapad/cunumeric that referenced this pull request Sep 7, 2022

Fix error and warning message when --launcher is missing (nv-legate#341)

a93ca89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing #321 issue #341

Fixing #321 issue #341

ipdemes commented May 10, 2022

magnatelee May 10, 2022

ipdemes May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

ipdemes May 10, 2022

magnatelee May 10, 2022

ipdemes May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

ipdemes May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

magnatelee May 10, 2022

manopapad commented May 11, 2022

manopapad commented May 12, 2022

ipdemes commented May 12, 2022

Fixing #321 issue #341

Fixing #321 issue #341

Conversation

ipdemes commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manopapad commented May 11, 2022

manopapad commented May 12, 2022

ipdemes commented May 12, 2022