Re: [Mesa-dev] [PATCH 12/13] clover: Add function for building a clover::module for non-TGSI targets v5

2012-05-28 Thread Francisco Jerez 
Tom Stellard tstel...@gmail.com writes:

 v2:
   -Separate IR type and LLVM triple
   -Do the OpenCL C-LLVM IR and linking steps for all PIPE_SHADER_IR
types.

 v3:
   - Coding style fixes
   - Removed compatibility code for LLVM  3.1
   - Split build_module_llvm() into three functions:
 compile(), link(), and build_module_llvm()

 v4:
   - Use struct pipe_compute_program

 v5:
   - Don't malloc memory for struct pipe_llvm_program
 ---
  .../state_trackers/clover/core/compiler.hpp|2 +
  src/gallium/state_trackers/clover/core/program.cpp |9 +-
  .../state_trackers/clover/llvm/invocation.cpp  |  165 
 ++--
  3 files changed, 162 insertions(+), 14 deletions(-)
[...]
 diff --git a/src/gallium/state_trackers/clover/core/program.cpp 
 b/src/gallium/state_trackers/clover/core/program.cpp
 index 06ac2af..8e34696 100644
 --- a/src/gallium/state_trackers/clover/core/program.cpp
 +++ b/src/gallium/state_trackers/clover/core/program.cpp
 @@ -47,9 +47,14 @@ _cl_program::build(const std::vectorclover::device * 
 devs) {
  
 for (auto dev : devs) {
try {
 - auto module = (dev-ir_target() == tgsi ?
 + // XXX: We need to check the input source to determine which
 + //  compile_program() call to use.  If the input is TGSI we
 + //  should use compile_program_tgsi, otherwise we should use
 + //  compile_program_llvm

I don't think we need to do that, we'll get rid of the support code for
compiling TGSI kernels once the CL-TGSI translation pass is ready.

 + auto module = (dev-ir_format() == PIPE_SHADER_IR_TGSI ?
  compile_program_tgsi(__source) :
 -compile_program_llvm(__source, dev-ir_target()));
 +compile_program_llvm(__source, dev-ir_format(),
 +  dev-ir_target()));
   __binaries.insert({ dev, module });
  
} catch (build_error e) {
 diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
 b/src/gallium/state_trackers/clover/llvm/invocation.cpp
 index 89e21bf..30bad7c 100644
 --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
 +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
[...]
 +  program.num_bytes = llvm_bitcode.size();
 +  std::string data;
 +  data.insert(0, (char*)(program.num_bytes), 4);

What's the point of defining the header layout using a struct if you're
pushing the header fields by hand into the object file?

 +  data.insert(data.end(), llvm_bitcode.begin(),
 +  llvm_bitcode.end());
 +  m.syms.push_back(module::symbol(kernel_name, 0, 0, args ));
 +  m.secs.push_back(module::section(0, module::section::text, data.size(),
 +   data));

This should pass 'llvm_bitcode.size()' instead of 'data.size()'.
'section::size' is supposed to be the virtual section size in memory,
excluding headers.

 +
 +  return m;
 +   }
 +} // End anonymous namespace
 +
[...]


pgpRxtu2WKihW.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 12/13] clover: Add function for building a clover::module for non-TGSI targets v5

2012-05-28 Thread Tom Stellard 
On Mon, May 28, 2012 at 10:03:27PM +0200, Francisco Jerez wrote:
 Tom Stellard tstel...@gmail.com writes:
 
  v2:
-Separate IR type and LLVM triple
-Do the OpenCL C-LLVM IR and linking steps for all PIPE_SHADER_IR
 types.
 
  v3:
- Coding style fixes
- Removed compatibility code for LLVM  3.1
- Split build_module_llvm() into three functions:
  compile(), link(), and build_module_llvm()
 
  v4:
- Use struct pipe_compute_program
 
  v5:
- Don't malloc memory for struct pipe_llvm_program
  ---
   .../state_trackers/clover/core/compiler.hpp|2 +
   src/gallium/state_trackers/clover/core/program.cpp |9 +-
   .../state_trackers/clover/llvm/invocation.cpp  |  165 
  ++--
   3 files changed, 162 insertions(+), 14 deletions(-)
 [...]
  diff --git a/src/gallium/state_trackers/clover/core/program.cpp 
  b/src/gallium/state_trackers/clover/core/program.cpp
  index 06ac2af..8e34696 100644
  --- a/src/gallium/state_trackers/clover/core/program.cpp
  +++ b/src/gallium/state_trackers/clover/core/program.cpp
  @@ -47,9 +47,14 @@ _cl_program::build(const std::vectorclover::device * 
  devs) {
   
  for (auto dev : devs) {
 try {
  - auto module = (dev-ir_target() == tgsi ?
  + // XXX: We need to check the input source to determine which
  + //  compile_program() call to use.  If the input is TGSI we
  + //  should use compile_program_tgsi, otherwise we should use
  + //  compile_program_llvm
 
 I don't think we need to do that, we'll get rid of the support code for
 compiling TGSI kernels once the CL-TGSI translation pass is ready.
 
  + auto module = (dev-ir_format() == PIPE_SHADER_IR_TGSI ?
   compile_program_tgsi(__source) :
  -compile_program_llvm(__source, dev-ir_target()));
  +compile_program_llvm(__source, dev-ir_format(),
  +dev-ir_target()));
__binaries.insert({ dev, module });
   
 } catch (build_error e) {
  diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
  b/src/gallium/state_trackers/clover/llvm/invocation.cpp
  index 89e21bf..30bad7c 100644
  --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
  +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
 [...]
  +  program.num_bytes = llvm_bitcode.size();
  +  std::string data;
  +  data.insert(0, (char*)(program.num_bytes), 4);
 
 What's the point of defining the header layout using a struct if you're
 pushing the header fields by hand into the object file?


I was trying to follow the suggestion you gave in the last email as
close as possible:

 It should be as simple as:

|header.num_bytes = llvm_bitcode.size();
|sec.data.insert(sec.data.end(), (char *)header,
|(char *)header + sizeof(header));
|sec.data.insert(sec.data.end(), llvm_bitcode.begin(),
|llvm_bitcode.end());

However, I realized that if you serialize it this way, you end up
serializing garbage for the second member of struct pipe_llvm_program,
which is a char*.  This also defeats the purpose of having a struct.

I've gone back and forth on this part several times, and I'm still not
sure what the correct solution is.  Do you have any other suggestions?

-Tom

  +  data.insert(data.end(), llvm_bitcode.begin(),
  +  llvm_bitcode.end());
  +  m.syms.push_back(module::symbol(kernel_name, 0, 0, args ));
  +  m.secs.push_back(module::section(0, module::section::text, 
  data.size(),
  +   data));
 
 This should pass 'llvm_bitcode.size()' instead of 'data.size()'.
 'section::size' is supposed to be the virtual section size in memory,
 excluding headers.
 
  +
  +  return m;
  +   }
  +} // End anonymous namespace
  +
 [...]




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

Re: [Mesa-dev] [PATCH 12/13] clover: Add function for building a clover::module for non-TGSI targets v5

2012-05-28 Thread Francisco Jerez 
Tom Stellard thomas.stell...@amd.com writes:

 On Mon, May 28, 2012 at 10:03:27PM +0200, Francisco Jerez wrote:
 Tom Stellard tstel...@gmail.com writes:
 
  v2:
-Separate IR type and LLVM triple
-Do the OpenCL C-LLVM IR and linking steps for all PIPE_SHADER_IR
 types.
 
  v3:
- Coding style fixes
- Removed compatibility code for LLVM  3.1
- Split build_module_llvm() into three functions:
  compile(), link(), and build_module_llvm()
 
  v4:
- Use struct pipe_compute_program
 
  v5:
- Don't malloc memory for struct pipe_llvm_program
  ---
   .../state_trackers/clover/core/compiler.hpp|2 +
   src/gallium/state_trackers/clover/core/program.cpp |9 +-
   .../state_trackers/clover/llvm/invocation.cpp  |  165 
  ++--
   3 files changed, 162 insertions(+), 14 deletions(-)
[...]
  diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
  b/src/gallium/state_trackers/clover/llvm/invocation.cpp
  index 89e21bf..30bad7c 100644
  --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
  +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
 [...]
  +  program.num_bytes = llvm_bitcode.size();
  +  std::string data;
  +  data.insert(0, (char*)(program.num_bytes), 4);
 
 What's the point of defining the header layout using a struct if you're
 pushing the header fields by hand into the object file?


 I was trying to follow the suggestion you gave in the last email as
 close as possible:

 It should be as simple as:

|header.num_bytes = llvm_bitcode.size();
|sec.data.insert(sec.data.end(), (char *)header,
|(char *)header + sizeof(header));
|sec.data.insert(sec.data.end(), llvm_bitcode.begin(),
|llvm_bitcode.end());

 However, I realized that if you serialize it this way, you end up
 serializing garbage for the second member of struct pipe_llvm_program,
 which is a char*.  This also defeats the purpose of having a struct.

I don't think it makes much sense to include that pointer in the struct,
if the only purpose is to represent an LLVM program header.

 I've gone back and forth on this part several times, and I'm still not
 sure what the correct solution is.  Do you have any other suggestions?

 -Tom



pgplQr2ltSGjj.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 12/13] clover: Add function for building a clover::module for non-TGSI targets v5

2012-05-28 Thread Tom Stellard 
On Mon, May 28, 2012 at 10:52:35PM +0200, Francisco Jerez wrote:
 Tom Stellard thomas.stell...@amd.com writes:
 
  On Mon, May 28, 2012 at 10:03:27PM +0200, Francisco Jerez wrote:
  Tom Stellard tstel...@gmail.com writes:
  
   v2:
 -Separate IR type and LLVM triple
 -Do the OpenCL C-LLVM IR and linking steps for all PIPE_SHADER_IR
  types.
  
   v3:
 - Coding style fixes
 - Removed compatibility code for LLVM  3.1
 - Split build_module_llvm() into three functions:
   compile(), link(), and build_module_llvm()
  
   v4:
 - Use struct pipe_compute_program
  
   v5:
 - Don't malloc memory for struct pipe_llvm_program
   ---
.../state_trackers/clover/core/compiler.hpp|2 +
src/gallium/state_trackers/clover/core/program.cpp |9 +-
.../state_trackers/clover/llvm/invocation.cpp  |  165 
   ++--
3 files changed, 162 insertions(+), 14 deletions(-)
 [...]
   diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp 
   b/src/gallium/state_trackers/clover/llvm/invocation.cpp
   index 89e21bf..30bad7c 100644
   --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
   +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
  [...]
   +  program.num_bytes = llvm_bitcode.size();
   +  std::string data;
   +  data.insert(0, (char*)(program.num_bytes), 4);
  
  What's the point of defining the header layout using a struct if you're
  pushing the header fields by hand into the object file?
 
 
  I was trying to follow the suggestion you gave in the last email as
  close as possible:
 
  It should be as simple as:
 
 |header.num_bytes = llvm_bitcode.size();
 |sec.data.insert(sec.data.end(), (char *)header,
 |(char *)header + sizeof(header));
 |sec.data.insert(sec.data.end(), llvm_bitcode.begin(),
 |llvm_bitcode.end());
 
  However, I realized that if you serialize it this way, you end up
  serializing garbage for the second member of struct pipe_llvm_program,
  which is a char*.  This also defeats the purpose of having a struct.
 
 I don't think it makes much sense to include that pointer in the struct,
 if the only purpose is to represent an LLVM program header.
 

OK, I get it now.  I'll redefine the header as:

struct pipe_llvm_program {
   uint32_t num_bytes;
}

Then I'll just serialize the bytecode after the header.

-Tom

  I've gone back and forth on this part several times, and I'm still not
  sure what the correct solution is.  Do you have any other suggestions?
 
  -Tom
 




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