[PATCH] D82451: [AMDGPU] Fix DPP Combiner:

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:358
+// Constrain the number of found Uses by it's preallocated size
+template <unsigned N>
+bool collectIndependSubRegUse(MachineRegisterInfo &MRI, RegSubRegPair RSR,
----------------
Usually for an output small vector, you pass SmallVectorImpl<Type>& rather than needing a template parameter for the size


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:359
+template <unsigned N>
+bool collectIndependSubRegUse(MachineRegisterInfo &MRI, RegSubRegPair RSR,
+                              SmallVector<MachineOperand *, N> &Uses) {
----------------
Independ looks like a weird abbrevation to me. Shorten to Indep or lengthen to Independent?


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:361
+                              SmallVector<MachineOperand *, N> &Uses) {
+  auto &TRI = *MRI.getTargetRegisterInfo();
+  LaneBitmask Mask = TRI.getSubRegIndexLaneMask(RSR.SubReg);
----------------
Should cast to const SIRegisterInfo


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:364
+  for (auto &Op : MRI.use_nodbg_operands(RSR.Reg)) {
+    if (Uses.size() == N)
+      return false;
----------------
It's weird to limit this based on the size of the small vector. It might make sense to add a maximum use to check, but disconnected from the vector size


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:376
+// Constrain the number of found Uses by it's preallocated size
+template <unsigned N>
+bool collectRegUse(MachineRegisterInfo &MRI, Register R,
----------------
Same as previous function


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:401
+
+template <typename R> MachineInstr *findInstrWithMoreThanOneUse(R &&Uses) {
+  // Assuming per instruction uses come together,
----------------
Why is this an rvalue reference? I also don't see why this needs to be a template


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:404-410
+  MachineInstr *PrevMI = nullptr;
+  for (auto &Use : Uses) {
+    auto *MI = Use->getParent();
+    if (MI == PrevMI)
+      return MI;
+    PrevMI = MI;
+  }
----------------
Why does this need to be a separate function? Can't you determine this over the initial use walk in collectRegUse?


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:493
+        dbgs() << "  failed: " << printReg(DPPMovReg)
+               << " either has too much uses or is used as part of a superreg");
+    return false;
----------------
s/much/many and end with newline


================
Comment at: llvm/lib/Target/AMDGPU/GCNDPPCombine.cpp:583
+  // Don't delete the original dpp mov instruction because it may still be
+  // used by a REG_SEQUENCE(potentialy orphane) producing invalid MIR. It will
+  // soon be deleted by the DCE.
----------------
Typo orphane


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82451