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

[Nicolai Hähnle]

On 22.05.2017 12:18, Marek Olšák wrote:
> 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.

Okay, that answers my question. Please add this explanation about glc=1, 
coherent=false to the comment in the function. For the series:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


> 
> Marek
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.