[PATCH] D87045: [LoopNest] Handle loop-nest passes in LoopPassManager

[Ettore Tiotto via Phabricator via llvm-commits]

etiotto added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopPassManager.h:65
 
+namespace detail {
+
----------------
Not sure if you can use an anonymous namespace


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:29
+  // Runs loop-nest passes only when the current loop is a top-level one.
+  if (!L.getParentLoop() && !LoopNestPasses.empty())
+    PA = runWithLoopNestPasses(L, AM, AR, U);
----------------
Suggest:
```
PreservedAnalyses PA = (!L.getParentLoop() && !LoopNestPasses.empty()) 
                                       ? runWithLoopNestPasses(L, AM, AR, U)
                                       : runWithoutLoopNestPasses(L, AM, AR, U);
```

That way PA is initialized where it is declared.


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:53
+                                       LPMUpdater &U) {
+  PreservedAnalyses PA = PreservedAnalyses::all();
+
----------------
assert L is the outermost loop (no parent loop).


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:65
+    Optional<PreservedAnalyses> PassPA;
+    if (!PassCategories.test(I)) {
+      auto &Pass = LoopPasses[LoopPassIndex++];
----------------
This isn't that clear, perhaps use something like `!isa<LoopNestPass>(Pass)` ?


================
Comment at: llvm/lib/Transforms/Scalar/LoopPassManager.cpp:119
+  for (auto &Pass : LoopPasses) {
+    auto PassPA = runSinglePass(L, Pass, AM, AR, U, PI);
+    if (!PassPA)
----------------
Is not immediately clear what is the type of PassPA. Could you use the explicit type `Optional<PreservedAnalyses>` ? 


================
Comment at: llvm/unittests/Transforms/Scalar/LoopPassManagerTest.cpp:1599
+
+TEST_F(LoopPassManagerTest, HandleLoopNestPass) {
+  ::testing::InSequence MakeExpectationsSequenced;
----------------
`HandleLoopNestPass` isn't used. How does this test work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87045