[Mesa-dev] [PATCH 1/3] gallivm: Don't use raw_debug_ostream for dissasembling

[Tom Stellard]

On Wed, Jul 29, 2015 at 09:54:05AM +0100, Jose Fonseca wrote:
> On 23/07/15 17:06, Jose Fonseca wrote:
> > On 20/07/15 21:39, Jose Fonseca wrote:
> >> On 20/07/15 18:35, Tom Stellard wrote:
> >>> All LLVM API calls that require an ostream object have been removed from
> >>> the disassemble() function, so we don't need to use this class to wrap
> >>> _debug_printf() we can just call this function directly.
> >>> ---
> >>>   src/gallium/auxiliary/gallivm/lp_bld_debug.cpp | 27
> >>> +++++++++++++-------------
> >>>   1 file changed, 13 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
> >>> b/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
> >>> index 405e648..ec88f33 100644
> >>> --- a/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
> >>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_debug.cpp
> >>> @@ -123,7 +123,7 @@ lp_debug_dump_value(LLVMValueRef value)
> >>>    * - http://blog.llvm.org/2010/04/intro-to-llvm-mc-project.html
> >>>    */
> >>>   static size_t
> >>> -disassemble(const void* func, llvm::raw_ostream & Out)
> >>> +disassemble(const void* func)
> >>>   {
> >>>      const uint8_t *bytes = (const uint8_t *)func;
> >>>
> >>> @@ -141,7 +141,8 @@ disassemble(const void* func, llvm::raw_ostream &
> >>> Out)
> >>>      char outline[1024];
> >>>
> >>>      if (!D) {
> >>> -      Out << "error: couldn't create disassembler for triple " <<
> >>> Triple << "\n";
> >>> +      _debug_printf("error: couldn't create disassembler for triple
> >>> %s\n",
> >>> +                    Triple.c_str());
> >>>         return 0;
> >>>      }
> >>>
> >>> @@ -155,13 +156,13 @@ disassemble(const void* func, llvm::raw_ostream
> >>> & Out)
> >>>          * so that between runs.
> >>>          */
> >>>
> >>> -      Out << llvm::format("%6lu:\t", (unsigned long)pc);
> >>> +      _debug_printf("%6lu:\t", (unsigned long)pc);
> >>>
> >>>         Size = LLVMDisasmInstruction(D, (uint8_t *)bytes + pc, extent
> >>> - pc, 0, outline,
> >>>                                      sizeof outline);
> >>>
> >>>         if (!Size) {
> >>> -         Out << "invalid\n";
> >>> +         _debug_printf("invalid\n");
> >>>            pc += 1;
> >>>            break;
> >>>         }
> >>> @@ -173,10 +174,10 @@ disassemble(const void* func, llvm::raw_ostream
> >>> & Out)
> >>>         if (0) {
> >>>            unsigned i;
> >>>            for (i = 0; i < Size; ++i) {
> >>> -            Out << llvm::format("%02x ", bytes[pc + i]);
> >>> +            _debug_printf("%02x ", bytes[pc + i]);
> >>>            }
> >>>            for (; i < 16; ++i) {
> >>> -            Out << "   ";
> >>> +            _debug_printf("   ");
> >>>            }
> >>>         }
> >>>
> >>> @@ -184,9 +185,9 @@ disassemble(const void* func, llvm::raw_ostream &
> >>> Out)
> >>>          * Print the instruction.
> >>>          */
> >>>
> >>> -      Out << outline;
> >>> +      _debug_printf("%*s", Size, outline);
> >>>
> >>> -      Out << "\n";
> >>> +      _debug_printf("\n");
> >>>
> >>>         /*
> >>>          * Stop disassembling on return statements, if there is no
> >>> record of a
> >>> @@ -206,13 +207,12 @@ disassemble(const void* func, llvm::raw_ostream
> >>> & Out)
> >>>         pc += Size;
> >>>
> >>>         if (pc >= extent) {
> >>> -         Out << "disassembly larger than " << extent << "bytes,
> >>> aborting\n";
> >>> +         _debug_printf("disassembly larger than %ull bytes,
> >>> aborting\n", extent);
> >>>            break;
> >>>         }
> >>>      }
> >>>
> >>> -   Out << "\n";
> >>> -   Out.flush();
> >>> +   _debug_printf("\n");
> >>>
> >>>      LLVMDisasmDispose(D);
> >>>
> >>> @@ -229,9 +229,8 @@ disassemble(const void* func, llvm::raw_ostream &
> >>> Out)
> >>>
> >>>   extern "C" void
> >>>   lp_disassemble(LLVMValueRef func, const void *code) {
> >>> -   raw_debug_ostream Out;
> >>> -   Out << LLVMGetValueName(func) << ":\n";
> >>> -   disassemble(code, Out);
> >>> +   _debug_printf("%s:\n", LLVMGetValueName(func));
> >>> +   disassemble(code);
> >>>   }
> >>>
> >>>
> >>>
> >>
> >> Series looks good AFAICT.
> >>
> >> Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
> >
> > Tom,
> >
> > This broke the profile build.  I fixed with commit
> > d6b50ba980b733a82fefe2a0f115635a359c445f, but only then I realized that
> > this change actually causes regression of functionality.
> >
> > The debug stream was being used so that we would write the assembly to
> > /tmp/perf-XXXXX.map.asm files, so that we could later annotate the
> > assembly intrusctions with the profile hits (via the
> > bin/perf-annotate-jit).
> >
> > But now the assembly is being redirected into the stderr, hence
> > perf-annotate-jit won't get anything from /tmp/perf-XXXXX.map.asm.
> >
> > What was your motivation for this change? General cleanup, or is the
> > llvm::raw_ostream causing you problems?
> >
> > I think that, if I don't reinstate the debug_stream, then I'll need to
> > replace it with STL streams (std::stringstream and std::ostream).
> >
> > Jose
> 
> Ping?
> 
> I wanted to make sure I didn't cause you trouble, but if I don't get any 
> answer I'll just assume you don't care either way, and revert the 
> change, as it saves me the effort of trying to replace debug_stream with 
> something else.
> 

Sorry, I wrote a reply but I must not have sent it.  The goal of this patch
was to eliminate the use of C++ API features in order to reduce the chances of
the build breaking.  If it is causing problems for you, go ahead and revert it.

-Tom

> Jose
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev