[PATCH] D75533: [ARM][LowOverheadLoops] Handle reductions

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Big patch: this is just a first scan of the code, and a first round of nits. Now going to look again, to let things sink in.



================
Comment at: llvm/include/llvm/CodeGen/ReachingDefAnalysis.h:155
 
-  /// Search MBB for a definition of PhysReg and insert it into Defs. If no
-  /// definition is found, recursively search the predecessor blocks for them.
-  void getLiveOuts(MachineBasicBlock *MBB, int PhysReg, InstSet &Defs,
-                   BlockSet &VisitedBBs) const;
+  /// Search MBB for a definition of PhysReg and insert it into Incoming. If no
+  /// definition is found, recursively search the successor blocks for them.
----------------
you renamed this `Defs` to `Incoming`...


================
Comment at: llvm/include/llvm/CodeGen/ReachingDefAnalysis.h:157
+  /// definition is found, recursively search the successor blocks for them.
+  void getLiveOuts(MachineBasicBlock *MBB, int PhysReg, InstSet &Defs) const;
 
----------------
So rename this one too?


================
Comment at: llvm/include/llvm/CodeGen/ReachingDefAnalysis.h:233
+  /// definition is found, recursively search the predecessor blocks for them.
+  void getLiveOuts(MachineBasicBlock *MBB, int PhysReg, InstSet &Defs,
+                   BlockSet &VisitedBBs) const;
----------------
and here too?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:179
+  struct Reduction {
+    MachineInstr *Init;
+    MachineInstr &Copy;
----------------
Can you comment what these members are? I agree that most are self-explanatory, but I am e.g. interested in `Init`, and if it e.g. can be null (there is a check in the fixup function), and what the meaning is of that.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:554
+MachineInstr* LowOverheadLoop::getMergePredicate(MachineInstr *VPSEL) const {
+  unsigned VPRIdx = llvm::findFirstVPTPredOperandIdx(*VPSEL) + 1;
+  MachineInstr *Pred = RDA.getMIOperand(VPSEL, VPRIdx);
----------------
nit: perhaps an assert that VPSEL is a vpsel would be good.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:574
+    for (auto &MO : reverse(MI->uses())) {
+      if (!MO.isReg() || MO.getReg() == 0)
+        continue;
----------------
nit, I think nicer to read is:
  MO.getReg() == 0
-> 
  !MO.getReg().isValid()


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:582
+  auto FirstImmUse = [](MachineInstr *MI, int64_t Imm) {
+    for (auto &MO : MI->uses()) {
+      if (!MO.isImm())
----------------
Nit: just a bit shorter would be:

  for (auto &MO : MI->uses()) {
    if (MO.isImm() && MO.getImm() == Imm)
       return true;
  return false;


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:630
+  MachineInstr *VPSEL = *LiveOutUsers.begin();
+  if (VPSEL->getOpcode() != ARM::MVE_VPSEL)
+    return false;
----------------
nit: was just curious why we expect the first item in the set to be the vpsel. Can we rely on that with a set?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:644
+  // TODO: Support more operations that VADD.
+  switch (VCTP->getOpcode()) {
+  default:
----------------
Could this be a good candidate for a helper function in ARMBaseInstrInfo.h?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:661
+
+  // Check that the VORR is actually a VMOV.
+  MachineInstr *Copy = RDA.getMIOperand(VPSEL, 2);
----------------
I guess you mean VMOV can be an alias for VORR, which you're checking here?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:688
+  // is storing the reduction.
+  if (!Preheader)
+    return false;
----------------
Can or should this not be checked much earlier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75533