[PATCH] D99649: [ARM] Updates to arm-block-placement pass

[Dave Green via Phabricator via llvm-commits]

dmgreen removed a reviewer: samparker.
dmgreen added a comment.

Thanks for the patch. Looks like a nice improvement.



================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:78
+    return Preheader;
+  if (Preheader->pred_size() == 1 && containsWLS(*(Preheader->pred_begin())))
+    return *(Preheader->pred_begin());
----------------
It may be best to make sure that Preheader has a single successor too, to make sure it doesn't look at a completely different block.

`(Preheader->pred_begin())` doesn't need brackets.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:95
+  for (auto &Terminator : Preheader->terminators()) {
+    if (Terminator.getOpcode() != ARM::t2WhileLoopStartLR)
+      continue;
----------------
Does this need to loop any more? It's already found a ARM::t2WhileLoopStartLR.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:180
+    if (CanMove) {
+      // doesnt have to be Preheader, refers to any BB that contains
+      // t2WhileLoopStartLR
----------------
doesnt -> Doesn't

I'm not sure what this comment is adding. It's probably better to just rename Preheader to Predecessor, if the name is wrong.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:189
+
+void ARMBlockPlacement::processPostOrderLoops(MachineLoop *ML, bool &Changed) {
+  for (auto *InnerML : *ML) {
----------------
It's better to return a bool for Changed than take it by reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99649