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

[Axel Davy via Phabricator via llvm-commits]

axeldavy added inline comments.


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:210
+        *I = Elem & Remaining;
+        LaneBasis.push_back(NewElem);
+      }
----------------
vpykhtin wrote:
> 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]
What do you suggest to fix this problem ?


================
Comment at: lib/Target/AMDGPU/SIMachineScheduler.cpp:376
+
+#ifndef NDEBUG
+
----------------
vpykhtin wrote:
> #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
> LLVM_DUMP_METHOD
Why LLVM_DUMP_METHOD on an empty line ?


================
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);
----------------
vpykhtin wrote:
> const SmallVectorImpl<RegisterMaskPair>&
right, I missed that for this update. Will be for next time.


================
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);
----------------
vpykhtin wrote:
> const SmallVectorImpl<RegisterMaskPair>&
canno be const here because we remove things from Uses.


Repository:
  rL LLVM

https://reviews.llvm.org/D31124