[Mesa-dev] [PATCH] ac/nir: fix intrinsic names for atomic operations with LLVM 9+

[Erik Faye-Lund]

On Mon, 2019-04-08 at 12:39 +0200, Samuel Pitoiset wrote:
> On 4/8/19 12:32 PM, Erik Faye-Lund wrote:
> > On Mon, 2019-04-08 at 11:39 +0200, Samuel Pitoiset wrote:
> > > This fixes the following LLVM error when using
> > > RADV_DEBUG=checkir:
> > > Intrinsic name not mangled correctly for type arguments! Should
> > > be:
> > > llvm.amdgcn.buffer.atomic.add.i32
> > > i32 (i32, <4 x i32>, i32, i32, i1)*
> > > @llvm.amdgcn.buffer.atomic.add
> > > 
> > > The cmpswap operation still uses the old intrinsic.
> > > 
> > > Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> > > ---
> > >   src/amd/common/ac_nir_to_llvm.c | 32 +++++++++++++++++++++-----
> > > ---
> > > ---
> > >   1 file changed, 21 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/src/amd/common/ac_nir_to_llvm.c
> > > b/src/amd/common/ac_nir_to_llvm.c
> > > index 6739551ca26..cc819286c65 100644
> > > --- a/src/amd/common/ac_nir_to_llvm.c
> > > +++ b/src/amd/common/ac_nir_to_llvm.c
> > > @@ -1679,7 +1679,8 @@ static void visit_store_ssbo(struct
> > > ac_nir_context *ctx,
> > >   static LLVMValueRef visit_atomic_ssbo(struct ac_nir_context
> > > *ctx,
> > >                                         const nir_intrinsic_instr
> > > *instr)
> > >   {
> > > -	const char *name;
> > > +	const char *op;
> > > +	char name[64];
> > >   	LLVMValueRef params[6];
> > >   	int arg_count = 0;
> > >   
> > > @@ -1696,39 +1697,48 @@ static LLVMValueRef
> > > visit_atomic_ssbo(struct
> > > ac_nir_context *ctx,
> > >   
> > >   	switch (instr->intrinsic) {
> > >   	case nir_intrinsic_ssbo_atomic_add:
> > > -		name = "llvm.amdgcn.buffer.atomic.add";
> > > +		op = "add";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_imin:
> > > -		name = "llvm.amdgcn.buffer.atomic.smin";
> > > +		op = "smin";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_umin:
> > > -		name = "llvm.amdgcn.buffer.atomic.umin";
> > > +		op = "umin";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_imax:
> > > -		name = "llvm.amdgcn.buffer.atomic.smax";
> > > +		op = "smax";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_umax:
> > > -		name = "llvm.amdgcn.buffer.atomic.umax";
> > > +		op = "umax";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_and:
> > > -		name = "llvm.amdgcn.buffer.atomic.and";
> > > +		op = "and";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_or:
> > > -		name = "llvm.amdgcn.buffer.atomic.or";
> > > +		op = "or";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_xor:
> > > -		name = "llvm.amdgcn.buffer.atomic.xor";
> > > +		op = "xor";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_exchange:
> > > -		name = "llvm.amdgcn.buffer.atomic.swap";
> > > +		op = "swap";
> > >   		break;
> > >   	case nir_intrinsic_ssbo_atomic_comp_swap:
> > > -		name = "llvm.amdgcn.buffer.atomic.cmpswap";
> > > +		op = "cmpswap";
> > >   		break;
> > >   	default:
> > >   		abort();
> > >   	}
> > >   
> > > +	if (HAVE_LLVM >= 0x900 &&
> > > +	    instr->intrinsic != nir_intrinsic_ssbo_atomic_comp_swap) {
> > > +		snprintf(name, sizeof(name),
> > > +                         "llvm.amdgcn.buffer.atomic.%s.i32",
> > > op);
> > The indention here seems off, compared to the else-case... (tabs vs
> > spaces?)
> Will fix before pushing.
> > > +	} else {
> > > +		snprintf(name, sizeof(name),
> > > +			 "llvm.amdgcn.buffer.atomic.%s", op);
> > > +	}
> > > +

I'm not an expert on LLVM, but the code changes looks good to me, and
the reasoning makes sense. So if you want:

Reviewed-by: Erik Faye-Lund <erik.faye-lund at collabora.com>