[PATCH] D41651: AMDGPU: Add 32-bit constant address space

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

In https://reviews.llvm.org/D41651#988249, @mareko wrote:

> Here is why relying on metadata is OK.
>
> The behavior of 64-bit pointers:
>
> - If the address is in VGPRs and amdgpu.uniform is not dropped, you'll get readfirstlane and correct behavior.
> - If the address is in VGPRs and amdgpu.uniform is dropped by a random pass, you'll get SMEM opcodes reading descriptors from VGPRs, so you'll get an invalid binary without an error and a GPU hang.


Just to clarify, do you mean that the 64-bit pointer will be used in a VMEM load, which would (typically) load a descriptor into VGPRs and then that descriptor would be fed as VGPRs into a MUBUF or MIMG instruction? We should fix that and automatically introduce v_readfirstlanes. I actually thought I had fixed that bug at some point in the past, but maybe it has reappeared :/

I agree that unlike silent miscompilation, compile errors are something we could potentially live with today. But if moving to VMEM is too difficult, I think it would still be better to declare that CONSTANT_ADDRESS_32BIT can only be used with dynamically uniform addresses, and just introduce a v_readfirstlane regardless of whether the metadata is there.

>> The other question is, why do we need a new address space at all? Can't we synthesize an appropriate pointer via inttoptr casts? I believe this is what SCPC is doing.
> 
> The short story is: We should never use inttoptr if InstCombine can't remove it. inttoptr is unoptimizable by LLVM.

Hmm yeah, I vaguely remember this now, although I have to admit that I never properly understood the details. I suppose adding the address space is easier than fighting the fight at the generic LLVM level.


Repository:
  rL LLVM

https://reviews.llvm.org/D41651