On δΊ”, 2014-06-20 at 07:18 +0000, Yang, Rong R wrote:
> Two comments.
>
> -----Original Message-----
> From: Beignet [mailto:beignet-boun...@lists.freedesktop.org] On Behalf Of
> junyan...@inbox.com
> Sent: Wednesday, June 18, 2014 2:42 PM
> To: beignet@lists.freedesktop.org
> Cc: Junyan He
> Subject: [Beignet] [PATCH 3/5] Add %f and %c support for printf.
>
> From: Junyan He <junyan...@linux.intel.com>
>
> Add the %c and %f support for printf.
> Also add the int to float and int to char conversion.
> Some minor errors such as wrong index flags have been fixed.
>
> Signed-off-by: Junyan He <junyan...@linux.intel.com>
> ---
> backend/src/ir/printf.cpp | 69 +++++++++++++++----------------
> backend/src/ir/printf.hpp | 4 ++
> backend/src/llvm/llvm_printf_parser.cpp | 72
> +++++++++++++++++++++++++--------
> 3 files changed, 93 insertions(+), 52 deletions(-)
>
> diff --git a/backend/src/ir/printf.cpp b/backend/src/ir/printf.cpp index
> 0a943ac..4bd7f2d 100644
> --- a/backend/src/ir/printf.cpp
> +++ b/backend/src/ir/printf.cpp
> @@ -17,18 +17,18 @@
> */
>
> /**
> - * \file sampler.cpp
> + * \file printf.cpp
> *
> */
>
> #include <stdarg.h>
> #include "printf.hpp"
> -#include "ocl_common_defines.h"
>
> namespace gbe
> {
> namespace ir
> {
> +
> pthread_mutex_t PrintfSet::lock = PTHREAD_MUTEX_INITIALIZER;
>
> uint32_t PrintfSet::append(PrintfFmt* fmt, Unit& unit) @@ -43,35 +43,21
> @@ namespace gbe
> }
>
> /* Update the total size of size. */
> - sizeOfSize = slots.back()->state->out_buf_sizeof_offset
> - + getPrintfBufferElementSize(slots.size() - 1);
> + if (slots.size() > 0)
> + sizeOfSize = slots.back()->state->out_buf_sizeof_offset
> + + getPrintfBufferElementSize(slots.size() - 1);
>
> return (uint32_t)fmts.size();
> }
>
> - /* ugly here. We can not build the va_list dynamically:(
> - And I have tried
> - va_list arg; arg = some_ptr;
> - This works very OK on 32bits platform but can not even
> - pass the compiling in the 64bits platform.
> - sizeof(arg) = 4 in 32bits platform but
> - sizeof(arg) = 24 in 64bits platform.
> - We can not assume the platform here. */
> - void vfprintf_wrap(std::string& fmt, vector<int>& contents)
> - {
> - int* ptr = NULL;
> - size_t num = contents.size() < 32 ? contents.size() : 32;
> - ptr = (int *)calloc(32, sizeof(int)); //should be enough
> - for (size_t i = 0; i < num; i++) {
> - ptr[i] = contents[i];
> - }
> -
> - printf(fmt.c_str(), ptr[0], ptr[1], ptr[2], ptr[3], ptr[4], ptr[5],
> ptr[6], ptr[7],
> - ptr[8], ptr[9], ptr[10], ptr[11], ptr[12], ptr[13], ptr[14],
> ptr[15], ptr[16],
> - ptr[17], ptr[18], ptr[19], ptr[20], ptr[21], ptr[22], ptr[23],
> ptr[24], ptr[25],
> - ptr[26], ptr[27], ptr[28], ptr[29], ptr[30], ptr[31]);
> - free(ptr);
> - }
> +#define PRINT_SOMETHING(target_ty, conv) do { \
> + pf_str = pf_str + std::string(#conv); \
> + printf(pf_str.c_str(), \
> + ((target_ty *)((char *)buf_addr +
> slot.state->out_buf_sizeof_offset * \
> + global_wk_sz0 * global_wk_sz1 * global_wk_sz2)) \
> + [k*global_wk_sz0*global_wk_sz1 + j*global_wk_sz0 + i]); \
> + pf_str = ""; \
> + } while (0)
>
> void PrintfSet::outputPrintf(void* index_addr, void* buf_addr, size_t
> global_wk_sz0,
> size_t global_wk_sz1, size_t global_wk_sz2)
> @@ -79,15 +65,15 @@ namespace gbe
> LockOutput lock;
> size_t i, j, k;
> std::string pf_str;
> - vector<int>* contents = NULL;
> + int stmt = 0;
> +
> for (auto &pf : fmts) {
> for (i = 0; i < global_wk_sz0; i++) {
> for (j = 0; j < global_wk_sz1; j++) {
> for (k = 0; k < global_wk_sz2; k++) {
> - int flag = ((int *)index_addr)[k*global_wk_sz0*global_wk_sz1 +
> j*global_wk_sz0 + i];
> + int flag = ((int
> + *)index_addr)[stmt*global_wk_sz0*global_wk_sz1*global_wk_sz2 +
> + k*global_wk_sz0*global_wk_sz1 + j*global_wk_sz0 + i];
> if (flag) {
> pf_str = "";
> - contents = new vector<int>();
> for (auto &slot : pf) {
> if (slot.type == PRINTF_SLOT_TYPE_STRING) {
> pf_str = pf_str + std::string(slot.str); @@ -98,23
> +84,34 @@ namespace gbe
> switch (slot.state->conversion_specifier) {
> case PRINTF_CONVERSION_D:
> case PRINTF_CONVERSION_I:
> - contents->push_back(((int *)((char *)buf_addr +
> slot.state->out_buf_sizeof_offset
> - * global_wk_sz0 *
> global_wk_sz1 * global_wk_sz2))
> - [k*global_wk_sz0*global_wk_sz1 +
> j*global_wk_sz0 + i]);
> - pf_str = pf_str + std::string("%d");
> + PRINT_SOMETHING(int, %d);
> break;
> + case PRINTF_CONVERSION_C:
> + PRINT_SOMETHING(char, %c);
> + break;
> +
> + case PRINTF_CONVERSION_F:
> + case PRINTF_CONVERSION_f:
> + if (slot.state->conversion_specifier ==
> PRINTF_CONVERSION_F)
> + PRINT_SOMETHING(float, %F);
> + else
> + PRINT_SOMETHING(float, %f);
> + break;
> +
> default:
> assert(0);
> return;
> }
> }
>
> - vfprintf_wrap(pf_str, *contents);
> - delete contents;
> + if (pf_str != "") {
> + printf(pf_str.c_str());
> + }
> }
> }
> }
> }
> + stmt++;
> }
> }
> } /* namespace ir */
> diff --git a/backend/src/ir/printf.hpp b/backend/src/ir/printf.hpp index
> b49ad0d..7e8027c 100644
> --- a/backend/src/ir/printf.hpp
> +++ b/backend/src/ir/printf.hpp
> @@ -196,7 +196,11 @@ namespace gbe
> switch (slot->state->conversion_specifier) {
> case PRINTF_CONVERSION_I:
> case PRINTF_CONVERSION_D:
> + case PRINTF_CONVERSION_C:
> return (uint32_t)sizeof(int);
> >>>>> PRINTF_CONVERSION_C should be word.
because out_buf_sizeof_offset += ((sizeof_size + 3) / 4) * 4;
So the sizeof size is always 4 bytes aligned.
this can avoid unaligned write and low performance.
>
>
> + case PRINTF_CONVERSION_F:
> + case PRINTF_CONVERSION_f:
> + return (uint32_t)sizeof(float);
> default:
> break;
> }
> diff --git a/backend/src/llvm/llvm_printf_parser.cpp
> b/backend/src/llvm/llvm_printf_parser.cpp
> index fa06995..2ea72d9 100644
> --- a/backend/src/llvm/llvm_printf_parser.cpp
> +++ b/backend/src/llvm/llvm_printf_parser.cpp
> @@ -217,7 +217,7 @@ namespace gbe
> CONVERSION_SPEC_AND_RET('s', A)
> CONVERSION_SPEC_AND_RET('p', P)
>
> - // %% has been handled
> + // %% has been handled
>
> default:
> return -1;
> @@ -321,8 +321,7 @@ error:
> static map<CallInst*, PrintfSet::PrintfFmt*> printfs;
> int printf_num;
>
> - PrintfParser(void) : FunctionPass(ID)
> - {
> + PrintfParser(void) : FunctionPass(ID) {
> module = NULL;
> builder = NULL;
> intTy = NULL;
> @@ -333,8 +332,7 @@ error:
> printf_num = 0;
> }
>
> - ~PrintfParser(void)
> - {
> + ~PrintfParser(void) {
> for (auto &s : printfs) {
> delete s.second;
> s.second = NULL;
> @@ -344,10 +342,9 @@ error:
>
>
> bool parseOnePrintfInstruction(CallInst *& call);
> - int generateOneParameterInst(PrintfSlot& slot, Value& arg);
> + int generateOneParameterInst(PrintfSlot& slot, Value*& arg, Type*&
> + dst_type);
>
> - virtual const char *getPassName() const
> - {
> + virtual const char *getPassName() const {
> return "Printf Parser";
> }
>
> @@ -466,13 +463,17 @@ error:
>
> assert(i < static_cast<int>(call->getNumOperands()) - 1);
>
> - int sizeof_size = generateOneParameterInst(s, *call->getOperand(i));
> + Value *out_arg = call->getOperand(i);
> + Type *dst_type = NULL;
> + int sizeof_size = generateOneParameterInst(s, out_arg, dst_type);
> if (!sizeof_size) {
> printf("Printf: %d, parameter %d may have no result because some
> error\n",
> printf_num, i - 1);
> continue;
> }
>
> + assert(dst_type);
> +
> /////////////////////////////////////////////////////
> /* Calculate the data address.
> data_addr = data_offset + pbuf_ptr + offset * sizeof(specify) @@
> -485,10 +486,10 @@ error:
> //data_offset + pbuf_ptr
> op0 = builder->CreateAdd(op0, pbuf_ptr);
> op0 = builder->CreateAdd(op0, val);
> - data_addr = builder->CreateIntToPtr(op0,
> Type::getInt32PtrTy(module->getContext(), 1));
> - builder->CreateStore(call->getOperand(i), data_addr);
> + data_addr = builder->CreateIntToPtr(op0, dst_type);
> + builder->CreateStore(out_arg, data_addr);
> s.state->out_buf_sizeof_offset = out_buf_sizeof_offset;
> - out_buf_sizeof_offset += sizeof_size;
> + out_buf_sizeof_offset += ((sizeof_size + 3) / 4) * 4;
> i++;
> }
>
> @@ -597,27 +598,66 @@ error:
> return changed;
> }
>
> - int PrintfParser::generateOneParameterInst(PrintfSlot& slot, Value& arg)
> + int PrintfParser::generateOneParameterInst(PrintfSlot& slot, Value*&
> + arg, Type*& dst_type)
> {
> assert(slot.type == PRINTF_SLOT_TYPE_STATE);
> assert(builder);
>
> /* Check whether the arg match the format specifer. If needed, some
> conversion need to be applied. */
> - switch (arg.getType()->getTypeID()) {
> + switch (arg->getType()->getTypeID()) {
> case Type::IntegerTyID: {
> switch (slot.state->conversion_specifier) {
> case PRINTF_CONVERSION_I:
> case PRINTF_CONVERSION_D:
> /* Int to Int, just store. */
> + dst_type = Type::getInt32PtrTy(module->getContext(), 1);
> return sizeof(int);
> - break;
> +
> + case PRINTF_CONVERSION_C:
> + /* Int to Char, add a conversion. */
> + arg = builder->CreateIntCast(arg,
> Type::getInt8Ty(module->getContext()), false);
> + dst_type = Type::getInt8PtrTy(module->getContext(), 1);
> + return sizeof(char);
> +
> + case PRINTF_CONVERSION_F:
> + case PRINTF_CONVERSION_f:
> + arg = builder->CreateSIToFP(arg,
> Type::getFloatTy(module->getContext()));
> + dst_type = Type::getFloatPtrTy(module->getContext(), 1);
> + return sizeof(float);
>
> default:
> return 0;
> }
> +
> + break;
> + }
> +
> + case Type::DoubleTyID:
> + case Type::FloatTyID: {
> + /* Because the printf is a variable parameter function, it does not
> have the
> + function prototype, so the compiler will always promote the arg
> to the
> + longest precise type for float. So here, we can always find it is
> double. */
> + switch (slot.state->conversion_specifier) {
> + case PRINTF_CONVERSION_I:
> + case PRINTF_CONVERSION_D:
> + /* Float to Int, add a conversion. */
> + arg = builder->CreateFPToSI(arg,
> Type::getInt32Ty(module->getContext()));
> + dst_type = Type::getInt32PtrTy(module->getContext(), 1);
> + return sizeof(int);
> +
> >>>>> How about convert float to char?
Will be add next step.
>
>
>
>
> + case PRINTF_CONVERSION_F:
> + case PRINTF_CONVERSION_f:
> + arg = builder->CreateFPCast(arg,
> Type::getFloatTy(module->getContext()));
> + dst_type = Type::getFloatPtrTy(module->getContext(), 1);
> + return sizeof(float);
> +
> + default:
> + return 0;
> + }
> +
> + break;
> }
> - break;
>
> default:
> return 0;
> --
> 1.8.3.2
>
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
_______________________________________________
Beignet mailing list
Beignet@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/beignet
