[PATCH] D60459: SILoadStoreOptimizer pass schedules s_add, s_addc with interfering s_lshl

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:197
 
+  // used to extend addToListsIfDependent to express Bailing.
+  enum AddToStat {AddToTrue, AddToFalse, AddToBail };
----------------
Capitalize


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:201-202
+                                  DenseSet<unsigned> &RegDefs,
+                                      DenseSet<unsigned> &PhysRegUses,
+                                      SmallVectorImpl<MachineInstr *> &Insts);
+
----------------
Indentation


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:278-279
+  auto E = ++MachineBasicBlock::const_iterator(MI.getIterator()).getReverse();
+  for (auto I = B.rbegin(); I != E; ++I)
+    Regs.stepBackward(*I);
+}
----------------
This is going to do a ~full scan of the block for every analyzed instruction, so this ends up being O(N^2). I was thinking more a single LivePhysReg instance for the entire block visit, which is lazily moved to the current point as necessary


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:301
+  // Reg is live, does this instr define it?
+  if (I->definesRegister(Reg))
+    return &*I;
----------------
This may not be broad enough. It only covers full defs. Usually modifiesRegister is what you want


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:325
          (Use.isDef() && TargetRegisterInfo::isPhysicalRegister(Use.getReg()) &&
           PhysRegUses.count(Use.getReg())))) {
+      // If this MI depends on a physReg such as SCC, find and add defining
----------------
The idea with using LivePhysRegs is to stop using this custom PhysRegUses set


================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:328
+      // instr. If not found, bail on this optimization.
+      if (Use.isImplicit() &&
+          TargetRegisterInfo::isPhysicalRegister(Use.getReg())) {
----------------
The implicitness doesn't matter


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

https://reviews.llvm.org/D60459