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

[Malhar via Phabricator via llvm-commits]

malharJ marked an inline comment as not done.
malharJ added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:95
+  MachineInstr *wlsInstr = nullptr;
+  for (auto &Terminator : Predecessor->terminators()) {
+    if (Terminator.getOpcode() == ARM::t2WhileLoopStartLR)
----------------
dmgreen wrote:
> malharJ wrote:
> > dmgreen wrote:
> > > It still probably doesn't need to search for it. If we've found it once, we can just return the WLS instruction.
> > > 
> > > Also LLVM capitalizes variable names.
> > yeah, I thought I'd keep the interface a little clean by not returning a pair.
> > I've made the update now though.
> Maybe just return the MachineInstr and use Predecessor = WlsInstr->getParent()? I agree not using the pair is a little cleaner.
Ah, thanks.


================
Comment at: llvm/lib/Target/ARM/ARMBlockPlacement.cpp:78
+    return Preheader;
+  if (Preheader->pred_size() == 1 && containsWLS(*(Preheader->pred_begin())))
+    return *(Preheader->pred_begin());
----------------
dmgreen wrote:
> 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.
Can we leave it as getLoopPredecessor() for now, ie. avoiding the strict requirement of a single successor ?
This would be helpful in keeping it flexible when making the WLSTP transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99649