[PATCH] D98455: [Polly] Refactoring astScheduleDimIsParallel to take the C++ wrapper object. NFC
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 11 15:04:08 PST 2021
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
More information about the llvm-commits
mailing list