[PATCH] D51932: [AMDGPU] Fix-up cases where writelane has 2 SGPR operands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:798-799
+        // normally
+        // count as using the constant bus twice - but in this case it is
+        // allowed as the lane selector doesn't count as a use of the constant
+        // bus). However, it is still required to abide by the 1 SGPR rule Apply
----------------
I think there's a missing word here; " as the lane selector and doesn't"


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:800
+        // allowed as the lane selector doesn't count as a use of the constant
+        // bus). However, it is still required to abide by the 1 SGPR rule Apply
+        // a fix here as we might have multiple SGPRs after legalizing VGPRs to
----------------
Missing period


================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:816-818
+          // Check for trivially easy constant prop into one of the operands
+          // If this is the case then perform the operation now to resolve SGPR
+          // issue
----------------
Is this really necessary? Will SIFoldOperands not get this for some reason?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3438
+  if (Desc.getOpcode() == AMDGPU::V_WRITELANE_B32) {
+
+    unsigned SGPRCount = 0;
----------------
Extra empty line


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3440
+    unsigned SGPRCount = 0;
+    unsigned SGPRUsed = AMDGPU::NoRegister;
+
----------------
Should use Register


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:6494
+  if (Op.isReg()) {
+    for (const MachineOperand &Def : MRI->def_operands(Op.getReg())) {
+      if (!isSameReg(Op, Def))
----------------
I would expect this to handle only a single vreg def


Repository:
  rL LLVM

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

https://reviews.llvm.org/D51932