[PATCH] D93268: [OpenMPIRBuilder] Implement collapseLoops.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Cool, really happy to see this. I tried to follow the logic but had some questions. Most comments are nits though.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1166
+  BranchInst::Create(Target, Source);
+}
+
----------------
Nit: Assert message.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1174
+    redirectTo(Pred, NewTarget);
+}
+
----------------
Nit: `auto *Pred` at least.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1202
+      break;
+  }
+
----------------
Nit: Maybe `do ... while(Changed)` but that's just an idea.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1206
+  DeleteDeadBlocks(BBVec);
+}
+
----------------
Do we really need/want to do such a search here? We could leave them be I guess, or delete them as we make them dead?

---

Interesting choices for sizes of the set and vector, I'm curious why


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1243
+                                           {}, /*HasNUW=*/true);
+  }
+
----------------
Nit: Can we add a TODO saying that an OpenMP sanitizer could use overflow tracking multiplication here and report an error if it does overflow.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1302
+  ContinueBlock = nullptr;
+  ContinuePred = Innermost->getLatch();
+
----------------
How can ContinueBlock in 1297 be non-null? Am I reading this wrong or are we using ContinueBlock once and then ContinuePred for all other loops?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1315
+    ContinueBlock = nullptr;
+  }
+
----------------
Maybe we want to move this body into a helper, it appears twice, unclear if that helps.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1321
+  else
+    redirectAllPredecessorsTo(ContinuePred, Result->getLatch());
+
----------------
Again I don't see how ContinueBlock is non-null, actually, even 1309 it is always null, isn't it? Something seems to be off, maybe the two are swapped?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93268