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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added a comment.

In D124196#3829110 <https://reviews.llvm.org/D124196#3829110>, @ruiling wrote:

> AFAIK, the WWM register has some unmodeled liveness behavior, which makes it impossible to allocate wwm register together with normal vector register in one pass now.
> For example(a typical if-then):
>
>   bb0:
>     %0 = ...
>     s_cbranch_execz %bb2
>   
>   bb1:
>     %1 = wwm_operation
>     ... = %1
>     %0 = ...
>   
>   bb2:
>     ... = %0
>
> VGPR %0 was dead in `bb1` and WWM-VGPR %1 was defined and used in `bb1`. As there is no live-range conflict between them, they have a chance to get assigned the same physical register. If this happens, certain lane of %0 might be overwritten when writing to %1. I am not sure if moving the SIPreAllocateWWMRegs between the sgpr allocation and the vgpr allocation might help your case? The key point is to request the SIPreAllocateWWMRegs allocate the wwm register usage introduced in SILowerSGPRSpills.

Agree, allowing wwm-register allocation together with the regular vector registers would be error prone with miscomputed liveness data.
But I guess, they are edge cases . Unlike the other WWM operations, the writelane/readlane for SGPR spill stores/restores, in most cases, would span across different blocks and such a liveness miscomputation would be a rare combination.

IIRC, `SIPreAllocateWWMRegs` can help allocate only when we have enough free VGPRs. There is no live-range spill/split incorporated in this custom pass. It won’t help in the case of large functions with more SGPR spills.
The best approach would be to introduce another regalloc pipeline between the existing SGPR and VGPR allocations. The new pipeline should allocate only the WWM-registers. 
It would, however, increase the compile time complexity further. But I’m not sure we have a better choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124196