[PATCH] D30439: [AMDGPU] New method to estimate register pressure

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNSchedStrategy.cpp:396
+  const SIRegisterInfo *SRI = static_cast<const SIRegisterInfo*>(TRI);
+  SlotIndex SI = LIS->getInstructionIndex(*begin());
+  assert (SI.isValid());
----------------
Should this specify slot type explicitly? Something like LIS->getInstructionIndex(*begin()).getBaseIndex() ? I just don't know which slot will be returned by default. For example if it return dead slot you will get ranges that actually live after the instruction.


================
Comment at: lib/Target/AMDGPU/GCNSchedStrategy.cpp:415
+    if (LaneMask.any()) {
+      LiveIns[Reg] = LaneBitmask::getNone();
+      setMask(MRI, SRI, Reg, LiveIns[Reg], LaneMask, SGPRs, VGPRs);
----------------
When a map is indexed for non-existent key the value constructed with default constructor is inserted for this key. LaneBitmask::getNone ()is just a syntax sugar for LaneBitmask(). So you can just read LiveIns[Reg] here without initializing it.


================
Comment at: lib/Target/AMDGPU/GCNSchedStrategy.cpp:472
+      if (LiveRegs.find(Reg) == LiveRegs.end())
+        LiveRegs[Reg] = LaneBitmask::getNone();
+      setMask(MRI, SRI, Reg, LiveRegs[Reg], LiveRegs[Reg] | LaneMask,
----------------
Again this is redundant. If there is no such key in the map the LiveRegs[Reg] will return LaneBitmask() on read.


================
Comment at: lib/Target/AMDGPU/GCNSchedStrategy.cpp:473
+        LiveRegs[Reg] = LaneBitmask::getNone();
+      setMask(MRI, SRI, Reg, LiveRegs[Reg], LiveRegs[Reg] | LaneMask,
+              SGPRs, VGPRs);
----------------
Please don't copy/paste map indexing. You can't assume compiler will always optimize this and the operation involves hash calculation on the key and array indexing (in the best case). I don't encorage such copy/paste indexing even for ordinary arrays.


Repository:
  rL LLVM

https://reviews.llvm.org/D30439