[PATCH] D31124: AMDGPU/SI: Add lane tracking to SI Scheduler

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin added a comment.

First part of comments related only to C++ issues



================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:144
+    const SmallVector<RegisterMaskPair, 8> &LiveIns,
+    const SmallVector<RegisterMaskPair, 8> &LiveOuts,
+    const std::vector<SmallVector<unsigned, 8>> &ItemSuccs,
----------------
const SmallVectorImpl<RegisterMaskPair> &


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:196
+      LaneBitmask Remaining = LaneMask;
+      for (SmallVector<LaneBitmask, 8>::iterator I = LaneBasis.begin();
+           I != LaneBasis.end(); ++I) {
----------------
I recommend use auto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:210
+        *I = Elem & Remaining;
+        LaneBasis.push_back(NewElem);
+      }
----------------
You're iterating over a vector increasing its' size at the same time. This is almost ok, except that vector can reallocate and all iterators would become invalidated. 

Iterator Invalidation Rules (C++03)
vector: all iterators and references before the point of insertion are unaffected, unless the new container size is greater than the previous capacity (in which case all iterators and references are invalidated) [23.2.4.3/1]


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:217
+
+  auto convertRegs = [=](SmallVector<RegisterMaskPair, 8> Regs) {
+    return getPairsForRegs(Regs);
----------------
Use const SmallVectorImpl<RegisterMaskPair> &



================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:229
+  // Add InRegs to LiveRegs after we add missing LiveIns
+  SmallVector<RegisterMaskPair, 8> InRegs = getPairsForRegs(LiveIns);
+
----------------
auto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:247
+      for (unsigned PredID : ItemPreds[i]) {
+        const SmallVector<RegisterMaskPair, 8> &PredOutRegs =
+          this->OutRegsForItem[PredID];
----------------
auto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:294
+      int ID = TopoIndexToItem[DAGSize-1-i];
+      const SmallVector<RegisterMaskPair, 8> &OutRegs =
+        this->OutRegsForItem[ID];
----------------
auto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:325
+  SmallDenseMap<unsigned, LaneBitmask> Map;
+  SmallPtrSet<unsigned, 8> Set;
+
----------------
SmallPtrSet is designed for pointers, gcc would complain, try consider SmallDenseSet

Are you developing using MSVS?


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:344
+      PSetIterator PSetI = MRI->getPressureSets(RegPair.first);
+      for (; PSetI.isValid(); ++PSetI) {
+        if (*PSetI == VGPRSetID)
----------------
I saw a lot of such code snippets in your code, please make a function


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:376
+
+#ifndef NDEBUG
+
----------------
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:102
+
+  void getCurrentRegUsage(unsigned &VGPR, unsigned &SGPR);
+  // Check register pressure change
----------------
I think I should change unsiged to signed values inside GCNRegPressure structure so we could use it as a regpressure difference too.


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:123
+
+  void addLiveRegs(SmallVector<RegisterMaskPair, 8> &Regs);
+  void decreaseLiveRegs(SmallVector<RegisterMaskPair, 8> &Regs);
----------------
const SmallVectorImpl<RegisterMaskPair> &

In general, when using Small... containers use SmallContainerImpl<T>& for passing by reference, its designed to be used so.


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:124
+  void addLiveRegs(SmallVector<RegisterMaskPair, 8> &Regs);
+  void decreaseLiveRegs(SmallVector<RegisterMaskPair, 8> &Regs);
+  void releaseItemSuccs(unsigned ID);
----------------
ditto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:218
   void addSucc(SIScheduleBlock *Succ, SIScheduleBlockLinkKind Kind);
+  void addLiveIns(SmallVector<RegisterMaskPair, 8> Ins);
+  void addLiveOuts(SmallVector<RegisterMaskPair, 8> Outs);
----------------
const SmallVectorImpl<RegisterMaskPair>&


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:219
+  void addLiveIns(SmallVector<RegisterMaskPair, 8> Ins);
+  void addLiveOuts(SmallVector<RegisterMaskPair, 8> Outs);
 
----------------
ditto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:247
 
-  std::set<unsigned> &getInRegs() { return LiveInRegs; }
-  std::set<unsigned> &getOutRegs() { return LiveOutRegs; }
+  SmallVector<RegisterMaskPair, 8> &getInRegs() { return LiveInRegs; }
+  SmallVector<RegisterMaskPair, 8> &getOutRegs() { return LiveOutRegs; }
----------------
**const **SmallVector<RegisterMaskPair, 8> &getInRegs() **const** { return LiveInRegs; }


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:248
+  SmallVector<RegisterMaskPair, 8> &getInRegs() { return LiveInRegs; }
+  SmallVector<RegisterMaskPair, 8> &getOutRegs() { return LiveOutRegs; }
 
----------------
ditto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:387
+  LaneBitmask getLaneBitmaskForUse(const SUnit *SU, unsigned Reg);
+  void removeUseFromDef(SmallVector<RegisterMaskPair, 8> &Uses,
+                        unsigned Reg, const SUnit *SU);
----------------
const SmallVectorImpl<RegisterMaskPair>&


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:389
+                        unsigned Reg, const SUnit *SU);
+  void addDefFromUse(SmallVector<RegisterMaskPair, 8> &Defs, unsigned Reg,
+                     const SUnit *SUDef, const SUnit *SUUse);
----------------
ditto


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:520
 
-  void restoreSULinksLeft();
-
-  template<typename _Iterator> void fillVgprSgprCost(_Iterator First,
-                                                     _Iterator End,
-                                                     unsigned &VgprUsage,
-                                                     unsigned &SgprUsage);
-
-  std::set<unsigned> getInRegs() {
-    std::set<unsigned> InRegs;
-    for (const auto &RegMaskPair : RPTracker.getPressure().LiveInRegs) {
-      InRegs.insert(RegMaskPair.RegUnit);
-    }
-    return InRegs;
+  const SmallVector<RegisterMaskPair, 8> &getInRegs() {
+    return RPTracker.getPressure().LiveInRegs;
----------------
const SmallVector<RegisterMaskPair, 8> &getInRegs() const


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.h:524
 
-  std::set<unsigned> getOutRegs() {
-    std::set<unsigned> OutRegs;
-    for (const auto &RegMaskPair : RPTracker.getPressure().LiveOutRegs) {
-      OutRegs.insert(RegMaskPair.RegUnit);
-    }
-    return OutRegs;
+  const SmallVector<RegisterMaskPair, 8> &getOutRegs() {
+    return RPTracker.getPressure().LiveOutRegs;
----------------
ditto


Repository:
  rL LLVM

https://reviews.llvm.org/D31124