[PATCH] D84391: [AMDGPU] Fix incorrect arch assert while setting up FlatScratchInit

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added a comment.

In D84391#2170772 <https://reviews.llvm.org/D84391#2170772>, @scott.linder wrote:

> In D84391#2170741 <https://reviews.llvm.org/D84391#2170741>, @scott.linder wrote:
>
> > Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were `flatScratchIsHwreg()` and it implied `flatScratchIsPointer()` this becomes:
> >
> >   if (ST.flatScratchIsHwreg()) {
> >     // pointer add then S_SETREG
> >   } else if (ST.flatScratchIsPointer()) (
> >     // pointer add
> >   } else {
> >     // size and offset dance
> >   }
> >   llvm_unreachable();
> >
> >
> > I agree that the existing assert here doesn't really mean anything, not sure why I wrote it.
>
>
> Rather, the `llvm_unreachable` isn't needed if we consider the offset/add case as the "default", but maybe we should predicate it too? It would be nice if we could do this all in a way that was statically known to cover all the targets we have.
>
> Could we combine the various predicate functions into one which returns an enumeration instead? Similar to Madhur's proposal, but without the direct reference to GFX#? I.e.
>
>   switch (ST.getFlatScratchSetupKind()) {
>   AMDGPU::PointerInHwreg:
>     // ...
>     break;
>   AMDGPU::PointerInSGPR:
>     // ...
>     break;
>   AMDGPU::SizeAndOffsetInSGPR:
>     // ...
>     break;
>   }
>
>
> Then when we add a new variant the compiler points us to everywhere that cares.


Yeah, let's handle this in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84391