[PATCH] D95071: [AMDGPU] Fix the inconsistency in soffset for MUBUF stack accesses.

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:495-502
 
   if (IsFlat) {
     assert(TII->isLegalFLATOffset(NewOffset, AMDGPUAS::PRIVATE_ADDRESS, true) &&
            "offset should be legal");
     FIOp->ChangeToRegister(BaseReg, false);
     OffsetOp->setImm(NewOffset);
     return;
----------------
scott.linder wrote:
> Not exactly related to this patch, but I feel like I must be reading this wrong. Why is there a different assert and an early return for DEBUG builds? If isLegalFLATOffset implies isLegalMUBUFImmOffset then this is just an additional, stricter check to what is below, but it still seems odd to have a `return` here then. Why not just fall through in the DEBUG case instead of copy-pasting the code?
The flat scratch code was an add-on to the existing code that earlier handled MUBUF instructions alone. Looks like the early return was meant to skip the soffset validation.


================
Comment at: llvm/test/CodeGen/AMDGPU/local-stack-alloc-block-sp-reference.ll:19
 
-define amdgpu_kernel void @local_stack_offset_uses_sp(i64 addrspace(1)* %out, i8 addrspace(1)* %in) {
 ; MUBUF-LABEL: local_stack_offset_uses_sp:
----------------
scott.linder wrote:
> I don't see the connection to the patch at hand, is this just unrelated cleanup or does this patch affect the dead code here?
This is completely an unrelated cleanup. This test was added as part of D87472. I noticed the dead argument and thought I can remove it.
Let me know if this should go as a separate NFC commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95071