[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:243-245
+    isl_union_map *RedDeps2 =
+        isl_union_map_from_map(isl_map_copy(MaRedPair.second));
+    if (!D->isParallel(Schedule.get(), RedDeps2))
----------------
QwertycowMoo wrote:
> Meinersbur wrote:
> > 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.
> > 
> 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?
> 
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.


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

https://reviews.llvm.org/D98455