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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: polly/lib/CodeGen/IslAst.cpp:209
                                      IslAstUserPayload *NodeInfo) {
+
   if (!D->hasValidDependences())
----------------
[nit] unrelated whitespace change


================
Comment at: polly/lib/CodeGen/IslAst.cpp:222
+    isl::pw_aff MinimalDependenceDistance;
+    // We will need to change isParallel to stop the unwrapping
+    isl_pw_aff *MinimalDependenceDistanceIsl =
----------------
Could you mark such line with an "TODO"?
```
// TODO: We will need to change isParallel to stop the unwrapping.
```


================
Comment at: polly/lib/CodeGen/IslAst.cpp:223-224
+    // We will need to change isParallel to stop the unwrapping
+    isl_pw_aff *MinimalDependenceDistanceIsl =
+        MinimalDependenceDistance.release();
+    D->isParallel(Schedule.get(), DepsAll.release(),
----------------
`MinimalDependenceDistance` contains nullptr at this point, not necessary to wrap it. Which basically means without changing `isParallel`, there is nothing to do here.


================
Comment at: polly/lib/CodeGen/IslAst.cpp:233-235
+  if (!D->isParallel(Schedule.get(), RedDeps.release())) {
     NodeInfo->IsReductionParallel = true;
+  }
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | LLVM's coding style does not use braces around single statements ]].
(it's also an unrelated change)


================
Comment at: polly/lib/CodeGen/IslAst.cpp:243-245
+    isl_union_map *RedDeps2 =
+        isl_union_map_from_map(isl_map_copy(MaRedPair.second));
+    if (!D->isParallel(Schedule.get(), RedDeps2))
----------------
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98455/new/

https://reviews.llvm.org/D98455