[Beignet] [Patch v2 2/2] [OCL20] atomic_flag_test_and_set function fix.

[Song, Ruiling]

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> xionghu.luo at intel.com
> Sent: Friday, March 11, 2016 11:42 PM
> To: beignet at lists.freedesktop.org
> Cc: Luo, Xionghu <xionghu.luo at intel.com>
> Subject: [Beignet] [Patch v2 2/2] [OCL20] atomic_flag_test_and_set function fix.
> 
> From: Luo Xionghu <xionghu.luo at intel.com>
> 
> should call atomic_compare_exchange_strong instead of the gen type call.
> 
> v2: still use __gen_ocl_atomic_compare_exchange_strong32 to implement
> the atomic_flag_test_and_set function as the spec says:
> "Returns atomically, the value of the object immediately
> before the effects."
> so if the flag old value is 0, should return 0 after calling
> atomic_flag_test_and_set, if the flag old value is 1 should return 1.
> secondly, cmpxchg instruction return a struct of {i32, i1}, also handle it
> in gen backend.
> this patch could fix the latest atomic_flag subset of conformance test.
> ---
>  backend/src/libocl/src/ocl_atom.cl    | 24 +++++++++++++++++++++---
>  backend/src/llvm/llvm_gen_backend.cpp |  7 +++++--
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/backend/src/libocl/src/ocl_atom.cl
> b/backend/src/libocl/src/ocl_atom.cl
> index e0af560..b49aef3 100644
> --- a/backend/src/libocl/src/ocl_atom.cl
> +++ b/backend/src/libocl/src/ocl_atom.cl
> @@ -331,17 +331,35 @@ DECL_ATOMIC_LOAD_TYPE(load_explicit, fetch_addf,
> atomic_float, atomic_int, float
> 
>  OVERLOADABLE bool atomic_flag_test_and_set(volatile atomic_flag *object) {
>    atomic_int * temp = (atomic_int*)object;
> -  return (bool)__gen_ocl_atomic_compare_exchange_strong32(temp, 0, 1,
> memory_order_seq_cst, memory_order_seq_cst, memory_scope_device);
> +  int expected = 0;
> +  int new_value = 1;
> +  int oldValue = __gen_ocl_atomic_compare_exchange_strong32(temp,
> expected, new_value, memory_order_seq_cst, memory_order_seq_cst,
> memory_scope_device);


You can use "return oldValue == new_value;" which is more concise.
And the same problem with below two changes.

> +  if(oldValue == new_value)
> +    return true;
> +  else
> +    return false;
>  }
> 
>  OVERLOADABLE bool atomic_flag_test_and_set_explicit(volatile atomic_flag
> *object, memory_order order) {
>    atomic_int * temp = (atomic_int*)object;
> -  return (bool)__gen_ocl_atomic_compare_exchange_strong32(temp, 0, 1,
> memory_order_seq_cst, memory_order_seq_cst, memory_scope_device);
> +  int expected = 0;
> +  int new_value = 1;
> +  int oldValue = __gen_ocl_atomic_compare_exchange_strong32(temp,
> expected, new_value, memory_order_seq_cst, memory_order_seq_cst,
> memory_scope_device);
> +  if(oldValue == new_value)
> +    return true;
> +  else
> +    return false;
>  }
> 
>  OVERLOADABLE bool atomic_flag_test_and_set_explicit(volatile atomic_flag
> *object, memory_order order, memory_scope scope){
>    atomic_int * temp = (atomic_int*)object;
> -  return (bool)__gen_ocl_atomic_compare_exchange_strong32(temp, 0, 1,
> memory_order_seq_cst, memory_order_seq_cst, memory_scope_device);
> +  int expected = 0;
> +  int new_value = 1;
> +  int oldValue = __gen_ocl_atomic_compare_exchange_strong32(temp,
> expected, new_value, memory_order_seq_cst, memory_order_seq_cst,
> memory_scope_device);
> +  if(oldValue == new_value)
> +    return true;
> +  else
> +    return false;
>  }
> 
>  OVERLOADABLE void atomic_flag_clear(volatile atomic_flag *object){
> diff --git a/backend/src/llvm/llvm_gen_backend.cpp
> b/backend/src/llvm/llvm_gen_backend.cpp
> index 6646536..de3aa4c 100644
> --- a/backend/src/llvm/llvm_gen_backend.cpp
> +++ b/backend/src/llvm/llvm_gen_backend.cpp
> @@ -3690,7 +3690,9 @@ namespace gbe
>      ir::AtomicOps opcode = ir::ATOMIC_OP_CMPXCHG;
>      uint32_t payloadNum = 0;
>      vector<ir::Register> payload;
> -    const ir::Register dst = this->getRegister(&I);
> +    const ir::Register oldValue = this->getRegister(&I, 0);
> +    const ir::Register compareRet = this->getRegister(&I, 1);
> +    const ir::Register expected = this->getRegister(I.getCompareOperand());
> 
>      payload.push_back(this->getRegister(I.getCompareOperand()));
>      payloadNum++;
> @@ -3700,7 +3702,8 @@ namespace gbe
>      const ir::Tuple payloadTuple = payloadNum == 0 ?
>                                     ir::Tuple(0) :
>                                     ctx.arrayTuple(&payload[0], payloadNum);
> -    this->emitAtomicInstHelper(opcode, type, dst, llvmPtr, payloadTuple);
> +    this->emitAtomicInstHelper(opcode, type, oldValue, llvmPtr, payloadTuple);
> +    ctx.EQ(type, compareRet, oldValue, expected);
>    }
> 
>    void GenWriter::regAllocateAtomicRMWInst(AtomicRMWInst &I) {
> --
> 2.1.4
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/beignet