[PATCH] D101830: AMDGPU: Correct const_index_stride for wave 32 for PAL API

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:551-553
+    // If we are in a wave32 shader we have to modify the const_index_stride to
+    // b10 We can't rely on the driver setting this for us since there are often
+    // multiple shaders with different wave sizes
----------------
dstuttard wrote:
> foad wrote:
> > The comment could be a bit more helpful. As I understand it: index_stride is a 2-bit field in bits 118:117 of the descriptor, so bits 22:21 of this word. (Is this the same for all architectures?) The value coming in might be either 0b10 (stride=32) or 0b11 (stride=64) so we force it to 0b10 by clearing bit 21.
> Yes - the driver will always set this up assuming wave64. Just setting the bits to the wave32 option covers that case (as well as the supposedly non-occurring case where the value is already correct for wave32).
> 
> How about:
> The driver will always set the SRD for wave 64. If the shader is actually wave32 we have to modify the const_index_stride field of the descriptor (bits 22:21) to b10 (stride=32). The reason the driver does this is that there can be cases where it presents 2 shaders with different wave size (e.g. VsFs). 
Sounds OK, though I think it would be better to mention the value 0b11 too, otherwise it's not obvious why just setting a single bit to 0 has the desired effect.

Also I've never seen binary literals with just a "b" prefix which is why I used "0b".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101830