[PATCH] D76708: [ARM][LowOverheadLoops] Add horizontal reduction support

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:609
   const TargetRegisterClass *QPRs = TRI.getRegClass(ARM::MQPRRegClassID);
   SetVector<MachineInstr *> Unknown;
   SmallPtrSet<MachineInstr *, 4> FalseLaneZeros;
----------------
Please add a comment that Unknown is the worklist of instructions that still need to be analysed, or something along those lines.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:635
+      if (retainsPreviousHalfElement(MI) || isHorizontalReduction(MI))
+        return false;
+      else
----------------
I had to think very hard again why we are returning false here (it's obvious now), but please add a comment to help the reader a bit here.

Or perhaps create a helper "MessingAroundWithTheLanes", which invokes retainsPreviousHalfElement and  isHorizontalReduction?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:669
   }
 
   // Collect Q-regs that are live in the exit blocks. We don't collect scalars
----------------
I was looking at `Unknown` again, and was wondering if it would be good to assert that Unknown should be empty at this point here. If I am not mistaken, that's what we expect here? That would require to remove items from Unknown where we insert it into to Predicated at line 667 above, but seems like a good check?


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

https://reviews.llvm.org/D76708