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

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:932
+  // Current score state. To speedup selection V2SCopyInfos for processing
+  bool IsVALU = false;
+  // Unique ID. Used as a key for mapping to keep permanent order.
----------------
a bit misleading name, maybe "ShouldGoToVALU"?


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:961
+  // and determines copy further lowering way: v_readfirstlane_b32 or moveToVALU
+  auto isVALU = [&](V2SCopyInfo* I) -> bool {
+    if (I->SChain.empty())
----------------
ditto


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:973
+      if (Copies.count(J)) {
+        MachineInstr * SiblingCopy = Copies[J].Copy;
+        if (SiblingCopy->isImplicitDef())
----------------
Copies.count(J) and Copies[J] lookups J twice


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1012
+        continue;
+      if (!LowerSpecialCase(MI)) {
+
----------------
if (LowerSpecialCase ,,, ) continue to reduce nesting


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1053
+            auto I = Inst->getIterator();
+            while((++I) != Inst->getParent()->end() &&
+              !I->findRegisterDefOperand(AMDGPU::SCC)) {
----------------
replace Inst->getParent()->end() with E


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1061
+            for (auto &U : MRI->use_instructions(Reg)) {
+              if (TRI->isSGPRReg(*MRI, Reg)) {
+                Users.push_back(&U);
----------------
why isSGPRReg is in the loop? It isn't being changed in the loop


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1086
+    if (Copies.count(CurID)) {
+      V2SCopyInfo C = Copies[CurID];
+      LLVM_DEBUG(dbgs() << "Processing ...\n"; C.dump());
----------------
Copies[CurID] and Copies.count(CurID) lookups for CurID twice


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1090
+        if (Copies.count(S)) {
+          V2SCopyInfo& SI = Copies[S];
+          LLVM_DEBUG(dbgs() << "Sibling:\n"; SI.dump());
----------------
ditto for S


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128252