[Mesa-dev] [PATCH 1/3] radeonsi: move building llvm.SI.load.const into ac_build_buffer_load

[Marek Olšák]

On Mon, May 22, 2017 at 8:44 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 19.05.2017 17:54, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> ---
>>  src/amd/common/ac_llvm_build.c           | 50
>> ++++++++++++++++++++++++--------
>>  src/amd/common/ac_llvm_build.h           |  3 +-
>>  src/amd/common/ac_nir_to_llvm.c          |  2 +-
>>  src/gallium/drivers/radeonsi/si_shader.c | 23 +++++++--------
>>  4 files changed, 51 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/amd/common/ac_llvm_build.c
>> b/src/amd/common/ac_llvm_build.c
>> index 87a1fb7..4b048ba 100644
>> --- a/src/amd/common/ac_llvm_build.c
>> +++ b/src/amd/common/ac_llvm_build.c
>> @@ -626,47 +626,73 @@ ac_build_buffer_store_dword(struct ac_llvm_context
>> *ctx,
>>  LLVMValueRef
>>  ac_build_buffer_load(struct ac_llvm_context *ctx,
>>                      LLVMValueRef rsrc,
>>                      int num_channels,
>>                      LLVMValueRef vindex,
>>                      LLVMValueRef voffset,
>>                      LLVMValueRef soffset,
>>                      unsigned inst_offset,
>>                      unsigned glc,
>>                      unsigned slc,
>> -                    bool readonly_memory)
>> +                    bool readonly_memory,
>> +                    bool coherent)
>
>
> I think the series is basically fine, but the use of "coherent" here is
> potentially confusing because "coherent" is already used as the memory
> qualifier in shaders, and it sets glc.
>
> Are there any buffer loads with readonly_memory && !glc && !vindex &&
> !voffset where we aren't allowed to use SMEM? If possible, we could just use
> that to control whether llvm.SI.load.const is used.
>
> I seem to recall that there were some subtle differences in bounds checking,
> and perhaps those prevent the use of SMEM sometimes. But then I'd rather use
> a more explicit parameter name like smem_forbidden or smem_allowed.

readonly_memory is now slightly misleading. It means that there are no
stores/atomics to the same memory in the current shader, and it
enforces READNONE. You need both non-coherent (stores in other shader
invocations, if any, are not coherent with the current shader) and
readonly_memory (no stores in the current shader touch the memory).
Note that stores in the current shader are always coherent, that's why
there must be none.

Also, relying on GLC would be wrong, because GLC is also used to work
around hw bugs, so GLC=1 can be set even when it's OK to use SMEM.

Marek