[PATCH] D69413: [AMDGPU] Fold AGPR reg_sequence initializers

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:691
+                        *MRI)) {
+        auto DL = UseMI->getDebugLoc();
+        auto MBB = UseMI->getParent();
----------------
const reference and no auto


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:697
+          UseMI->RemoveOperand(I);
+
+        DenseMap<TargetInstrInfo::RegSubRegPair, Register> VGPRCopies;
----------------
You can construct a MachineInstrBuilder for the existing instruction to replace the addOperands below with the less verbose versions


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:699
+        DenseMap<TargetInstrInfo::RegSubRegPair, Register> VGPRCopies;
+        SmallSetVector<TargetInstrInfo::RegSubRegPair, 32> SeenAGPRs;
+        for (unsigned I = 0; I < Size / 4; ++I) {
----------------
I think 32 is a bit big for a small set size


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:733
+            // create a copy here and track if we already have such a copy.
+            if (TRI->isSGPRReg(*MRI, Src.Reg) && Def->getParent()) {
+              CopyToVGPR = Src;
----------------
Why is this checking Def->getParent()? Why is an instruction under consideration that isn't inserted in a block?


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:750
+              BuildMI(*MBB, UseMI, DL, TII->get(AMDGPU::COPY), Vgpr)
+                ->addOperand(*Def);
+              VGPRCopies[CopyToVGPR] = Vgpr;
----------------
.add()


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

https://reviews.llvm.org/D69413