[PATCH] D63476: [ARM] DLS/LE low-overhead loop code generation

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

This looks pretty neat to me. I have some comments that could be done in followups (as in, if its broken, we can fix it later).

- It may be better to put this into constant island pass instead, to use its iterative nature and have it remain as the only place that deals with sizes.
- Can you put a nice long comment into somewhere like the start of ARMFinalizeHardwareLoops explaining things like what the pseudos do and why they are needed (they are for registry allocation, essentially?), and what assumptions this is making about them remaining in specific blocks.



================
Comment at: lib/Target/ARM/ARMInstrThumb2.td:5193
+  t2PseudoInst<(outs), (ins GPRlr:$elts, brtarget:$target),
+  8, IIC_Br, []>, Sched<[WriteBr]>;
+
----------------
12 bytes for something that should usually take 4 seems overly pessimistic to me, and may pessimise other optimisations. Can we get this smaller somehow? I would expect the fallback to probably be (subs; bne) ideally.


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:76
+  BBUtils = std::unique_ptr<ARMBasicBlockUtils>(new ARMBasicBlockUtils(MF));
+  BBUtils->computeAllBlockSizes();
+
----------------
How expensive is this? We might as well not do it for cores that won't have low overhead loops.


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:96
+
+  auto IsLoopStart = [](MachineInstr &MI) {
+    return MI.getOpcode() == ARM::t2DoLoopStart;
----------------
These don't really seem to add much to me :)


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:135
+      // If we find that we load/store LR between LoopDec and LoopEnd, revert
+      // back to a 'normal' loop.
+      if (MI.mayLoad() || MI.mayStore())
----------------
Can you explain this a little? Is it for correctness or performance? Because we can't merge the two instructions?


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:148
+  if (Start || Dec || End)
+    assert((Start && Dec && End) && "Failed to find all loop components");
+  else {
----------------
I think this should give a better error message in release builds. report_fatal_error or similar. Otherwise It may just miscompile. It should never happen, as far as I understand?


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:165
+  LLVM_DEBUG(dbgs() << "ARM Loops:\n - Found Loop Start: " << *Start
+             << " - Found Loop Dec: " << *Dec
+             << " - Found Loop End: " << *End);
----------------
Formatting


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:173
+void ARMLowOverheadLoops::Expand(MachineLoop *ML, MachineInstr *Start,
+                              MachineInstr *Dec, MachineInstr *End,
+                              bool Revert) {
----------------
Format


================
Comment at: lib/Target/ARM/ARMLowOverheadLoops.cpp:201
+    MachineInstrBuilder MIB = InsertPt ?
+      BuildMI(*MBB, InsertPt, Start->getDebugLoc(), TII->get(ARM::t2DLS)) :
+      BuildMI(*MBB, Start, Start->getDebugLoc(), TII->get(ARM::t2DLS));
----------------
This is just the same call, with one parameter different?


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

https://reviews.llvm.org/D63476