[PATCH] D104509: [RegisterCoalescer] Resolve conflict based on liveness of subregister

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:2859
+    auto &OtherLI = LIS->getInterval(Other.Reg);
+    bool ClobberOther = !OtherLI.hasSubRanges();
+    // If we are clobbering some active lanes of OtherVNI at VNI->def, it is
----------------
arsenm wrote:
> What's the point of entering the loop if this starts out as true? Can't you just early return here?
I have refined the logic for !OtherLI.hasSubranges() because I think they are still coalescable if there LaneMask don't have any common bit.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:2876-2879
+    if (ClobberOther)
+      return CR_Impossible;
+    else
+      return CR_Replace;
----------------
arsenm wrote:
> return ClobberOther ? CR_Impossible : CR_Replace
I have just moved "return CR_Impossible;" inside the loop.


================
Comment at: llvm/test/CodeGen/AMDGPU/splitkit-getsubrangeformask.ll:355
   ; CHECK:   [[V_OR_B32_e32_62:%[0-9]+]]:vgpr_32 = V_OR_B32_e32 [[V_OR_B32_e32_61]], [[V_ADD_U32_e32_26]], implicit $exec
-  ; CHECK:   [[SI_SPILL_S32_RESTORE1:%[0-9]+]]:sreg_32_xm0_xexec = SI_SPILL_S32_RESTORE %stack.0, implicit $exec, implicit $sgpr32 :: (load 4 from %stack.0, addrspace 5)
-  ; CHECK:   [[SI_SPILL_S128_RESTORE:%[0-9]+]]:sgpr_128 = SI_SPILL_S128_RESTORE %stack.1, implicit $exec, implicit $sgpr32 :: (load 16 from %stack.1, align 4, addrspace 5)
----------------
arsenm wrote:
> I'm a bit worried this test is no longer hitting what it was supposed to be testing
I have not check into details, I am not sure @foad Do you have any idea on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104509