[PATCH] D89170: [AMDGPU] Select flat scratch instructions where available

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:482
 
   if (MFI->hasFlatScratchInit()) {
     emitEntryFunctionFlatScratchInit(MF, MBB, I, DL, ScratchWaveOffsetReg);
----------------
rampitec wrote:
> Flakebi wrote:
> > Should this be `MFI->hasFlatScratchInit() || (ST.enableFlatScratch() && requiresStackPointerReference(MF))`?
> > Otherwise, the scratch does not get initialized (I guess it’s fine to do that in a later patch).
> Do you see it not initialized? In the SIMachineFunctionInfo() there is this code:
> 
> 
> ```
>   if (ST.hasFlatAddressSpace() && isEntryFunction() && isAmdHsaOrMesa) {
>     // TODO: This could be refined a lot. The attribute is a poor way of
>     // detecting calls or stack objects that may require it before argument
>     // lowering.
>     if (HasCalls || HasStackObjects)
>       FlatScratchInit = true;
>   }
> ```
> 
> So I assume it has to be initialized. Probably the culprit is this isAmdHsaOrMesa condition? It may be needed to say (isAmdHsaOrMesa || ST.enableFlatScratch()) instead. For some reason this code is not executed for amdpal, I do not see an obvious reason why.
Actually I see it uninitialized in my own test. But the code as suggested does not always work because requiresStackPointerReference() is not necessarily true if we have just private loads, we need to make sure hasFlatScratchInit() is set.


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

https://reviews.llvm.org/D89170