[PATCH] D128252: [AMDGPU] VGPR to SGPR copies lowering

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:99
+  void lowerVGPR2SGPRCopies(MachineFunction &MF);
+  bool LowerSpecialCase(MachineInstr& MI);
 
----------------
foad wrote:
> Likewise.
camelLowerCase the name.

Please also add a comment what special cases does it handle.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:924
+  DenseSet<MachineInstr*> SChain;
+  // Number of SGPR to VGPR copies that are used to put the SALU computation results back to VALU.
+  unsigned NumSVCopies;
----------------
Second to ask for a clang-format run.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:935
+  unsigned ID;
+  // Next unique ID to  use while new instance created.
+  static unsigned NextID;
----------------
Double space after "to".


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:945
+    NumReadfirstlanes(Width/32), ID(++NextID) {};
+  void dump() {
+    dbgs() << ID << " : "  << *Copy
----------------
`#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` around dump().



================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1029
+
+      SmallVector<MachineInstr *, 8> worklist;
+      // Needed because the SSA is not a tree but a graph and may have
----------------
Capitalize 'Worklist'.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1041
+
+        // Copies and REG_SEQUENCE do not comtribute to the final assembly
+        // So, skip them but take care of the SGPR to VGPR copies bookkeeping.
----------------
Typo "comtribute".


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1059
+          auto E = Inst->getParent()->end();
+          while((++I) != E &&
+            !I->findRegisterDefOperand(AMDGPU::SCC)) {
----------------
Why ++I is in parenthesis?


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1134
+      else
+        MIB.addReg(SrcReg);
+    } else {
----------------
What happens to 16 bit subregs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128252