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

[Bas Nieuwenhuizen]

r-b

On Mon, Apr 8, 2019 at 12:36 PM 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 
> >> ---
> >>   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);
> >> +}
> >> +
> >>  return ac_build_intrinsic(>ac, name, ctx->ac.i32, params,
> >> arg_count, 0);
> >>   }
> >>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
> > > ---
> > >   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 


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Pitoiset]

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 
---
  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);
+   }
+
return ac_build_intrinsic(>ac, name, ctx->ac.i32, params,
arg_count, 0);
  }
  

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Erik Faye-Lund]

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 
> ---
>  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?)

> + } else {
> + snprintf(name, sizeof(name),
> +  "llvm.amdgcn.buffer.atomic.%s", op);
> + }
> +
>   return ac_build_intrinsic(>ac, name, ctx->ac.i32, params,
> arg_count, 0);
>  }
>  

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Pitoiset]

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 
---
 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);
+   } else {
+   snprintf(name, sizeof(name),
+"llvm.amdgcn.buffer.atomic.%s", op);
+   }
+
return ac_build_intrinsic(>ac, name, ctx->ac.i32, params, 
arg_count, 0);
 }
 
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev