[Polly] Refactoring astScheduleDimIsParallel to take the C++ wrapper object. NFC

[QwertycowMoo]

Summary

Polly currently needs to be slowly refactor to use the C++ wrapper objects to handle the reference counters automatically.
I took the function of astScheduleDimIsParallel and refactored it so that it uses the C++ wrapper function as much as possible.

There are some problems with the IsParallel since it expects the C objects, so the C++ wrapper functions must be .release() and .get() first before they are able to be used with IsParallel.

When checking the ReductionDependencies Parallelism with the Build's Schedule, I opted to keep the union map as a C object rather than a C++ object. Eventually, changes will need to be made to IsParallel to refactor it to the C++ wrappers. When this is done, this function will also need to be slightly refactored to not use the C object.

[Meinersbur]

polly/lib/CodeGen/IslAst.cpp

209

[nit] unrelated whitespace change

222

Could you mark such line with an "TODO"?

// TODO: We will need to change isParallel to stop the unwrapping.

223–224

MinimalDependenceDistance contains nullptr at this point, not necessary to wrap it. Which basically means without changing isParallel, there is nothing to do here.

233–235

[style] LLVM's coding style does not use braces around single statements.
(it's also an unrelated change)

243–245

I used RedDeps2 in my examples, but its not a good variable name.

When checking the ReductionDependencies Parallelism with the Build's Schedule, I opted to keep the union map as a C object rather than a C++ object. Eventually, changes will need to be made to IsParallel to refactor it to the C++ wrappers. When this is done, this function will also need to be slightly refactored to not use the C object.

[suggestion] Consider

isl::union_map MaRedDeps = isl::manage_copy(MaRedPair.second);
if (!D->isParallel(Schedule.get(), MaRedDeps.release()))

I think it still looks better even without converting IsParallel as well.

[QwertycowMoo]

Edited whitespace and style conventions. Asked a question about the implicit conversion of isl::map to isl::union_map but still changed it.

[Meinersbur]

LGTM, thanks for the adjustments.

Would you me to push it to main?

[QwertycowMoo]

Yup! Please push for me I'm not sure whether I should be doing that yet so if you @Meinersbur could push it that would be great!

polly/lib/CodeGen/IslAst.cpp

243–245

Does this mean that we can have implicit conversion from isl::map to isl::union_map?
It seems that MaRedPair.second is an isl_map and when we apply isl::manage_copy we get an isl::map.
Can we implicitly convert that to isl::union_map?

[Meinersbur]

polly/lib/CodeGen/IslAst.cpp

243–245

Yes, this is what my suggested code is doing. Written with every conversion on its own line:

isl::map MaRedDeps1 = isl::manage_copy(MaRedPair.second);
isl::union_map MaRedDeps2 = MaRedDeps1; // implicit conversion
isl_union_map *MaRedDeps3 = MaRedDeps.release();
if (!D->isParallel(Schedule.get(), MaRedDeps3)

On a single line:

if (!D->isParallel(Schedule.get(), isl::union_map(isl::manage_copy(MaRedPair.second)).release())

where the conversion is not that implicit anymore.

polly/lib/CodeGen/IslAst.cpp

 /// In case the dimension is parallel we also check if any reduction
 /// dependences is broken when we exploit this parallelism. If so,
 /// @p IsReductionParallel will be set to true. The reduction dependences we use
 /// to check are actually the union of the transitive closure of the initial
 /// reduction dependences together with their reversal. Even though these
 /// dependences connect all iterations with each other (thus they are cyclic)
 /// we can perform the parallelism check as we are only interested in a zero
 /// (or non-zero) dependence distance on the dimension in question.
-static bool astScheduleDimIsParallel(__isl_keep isl_ast_build *Build,
+static bool astScheduleDimIsParallel(const isl::ast_build &Build,
                                      const Dependences *D,
                                      IslAstUserPayload *NodeInfo) {
   if (!D->hasValidDependences())
     return false;
 
-  isl_union_map *Schedule = isl_ast_build_get_schedule(Build);
-  isl_union_map *Deps =
-      D->getDependences(Dependences::TYPE_RAW | Dependences::TYPE_WAW |
-                        Dependences::TYPE_WAR)
-          .release();
+  isl::union_map Schedule = Build.get_schedule();
+  isl::union_map Dep = D->getDependences(
+      Dependences::TYPE_RAW | Dependences::TYPE_WAW | Dependences::TYPE_WAR);
 
-  if (!D->isParallel(Schedule, Deps)) {
-    isl_union_map *DepsAll =
+  if (!D->isParallel(Schedule.get(), Dep.release())) {
+    isl::union_map DepsAll =
         D->getDependences(Dependences::TYPE_RAW | Dependences::TYPE_WAW |
-                          Dependences::TYPE_WAR | Dependences::TYPE_TC_RED)
-            .release();
-    isl_pw_aff *MinimalDependenceDistance = nullptr;
-    D->isParallel(Schedule, DepsAll, &MinimalDependenceDistance);
+                          Dependences::TYPE_WAR | Dependences::TYPE_TC_RED);
+    // TODO: We will need to change isParallel to stop the unwrapping
+    isl_pw_aff *MinimalDependenceDistanceIsl = nullptr;
+    D->isParallel(Schedule.get(), DepsAll.release(),
+                  &MinimalDependenceDistanceIsl);
     NodeInfo->MinimalDependenceDistance =
-        isl::manage(MinimalDependenceDistance);
-    isl_union_map_free(Schedule);
+        isl::manage(MinimalDependenceDistanceIsl);
     return false;
   }
 
-  isl_union_map *RedDeps =
-      D->getDependences(Dependences::TYPE_TC_RED).release();
-  if (!D->isParallel(Schedule, RedDeps))
+  isl::union_map RedDeps = D->getDependences(Dependences::TYPE_TC_RED);
+  if (!D->isParallel(Schedule.get(), RedDeps.release()))
     NodeInfo->IsReductionParallel = true;
 
-  if (!NodeInfo->IsReductionParallel && !isl_union_map_free(Schedule))
+  if (!NodeInfo->IsReductionParallel)
     return true;
 
-  // Annotate reduction parallel nodes with the memory accesses which caused the
-  // reduction dependences parallel execution of the node conflicts with.
   for (const auto &MaRedPair : D->getReductionDependences()) {
     if (!MaRedPair.second)
       continue;
-    RedDeps = isl_union_map_from_map(isl_map_copy(MaRedPair.second));
-    if (!D->isParallel(Schedule, RedDeps))
+    isl::union_map MaRedDeps = isl::manage_copy(MaRedPair.second);
+    if (!D->isParallel(Schedule.get(), MaRedDeps.release()))
       NodeInfo->BrokenReductions.insert(MaRedPair.first);
   }
-
-  isl_union_map_free(Schedule);
   return true;
 }
 
 // This method is executed before the construction of a for node. It creates
 // an isl_id that is used to annotate the subsequently generated ast for nodes.
 //
 // In this function we also run the following analyses:
 //
 // - Detection of openmp parallel loops
 //
 static __isl_give isl_id *astBuildBeforeFor(__isl_keep isl_ast_build *Build,
                                             void *User) {
   AstBuildUserInfo *BuildInfo = (AstBuildUserInfo *)User;
   IslAstUserPayload *Payload = new IslAstUserPayload();
   isl_id *Id = isl_id_alloc(isl_ast_build_get_ctx(Build), "", Payload);
   Id = isl_id_set_free_user(Id, freeIslAstUserPayload);
   BuildInfo->LastForNodeId = Id;
 
-  Payload->IsParallel =
-      astScheduleDimIsParallel(Build, BuildInfo->Deps, Payload);
+  Payload->IsParallel = astScheduleDimIsParallel(isl::manage_copy(Build),
+                                                 BuildInfo->Deps, Payload);
 
   // Test for parallelism only if we are not already inside a parallel loop
   if (!BuildInfo->InParallelFor && !BuildInfo->InSIMD)
     BuildInfo->InParallelFor = Payload->IsOutermostParallel =
         Payload->IsParallel;
 
   return Id;
 }