[PATCH] D124196: [AMDGPU][SILowerSGPRSpills] Spill SGPRs to virtual VGPRs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

As a follow up I think we need to address the loss of being able to share VGPR lanes for unrelated spills



================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:264-265
+  // insert an IMPLICIT_DEF before the dominating spill. Switching to a
+  // depth first order doesn't really help since the machine function is in
+  // the the unstructured control flow post-SSA. For each virtual register,
+  // hence finding the common dominator to get either the dominating spill
----------------
Typo "the the". It's also not necessarily unstructured 


================
Comment at: llvm/lib/Target/AMDGPU/SILowerSGPRSpills.cpp:293
+        // the first spill in case of tuple spills.
+        if (!IsDominatesChecked) {
+          IsDominatesChecked = true;
----------------
IsDominatesChecked is confusingly named and expresses the code not the intent. SeenSpillInBlock?


================
Comment at: llvm/test/CodeGen/AMDGPU/need-fp-from-vgpr-spills.ll:202-204
+; CHECK-NEXT:    v_writelane_b32 v0, s30, 0
+; CHECK-NEXT:    v_writelane_b32 v0, s31, 1
+; CHECK-NEXT:    buffer_store_dword v0, off, s[0:3], s33 ; 4-byte Folded Spill
----------------
This is an unfortunate regression but what I expected


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196