[Beignet] [PATCH 5/6] Debug Support: Pass debug infomation to GenInsn

[Song, Ruiling]

Add one more comment from Nanhai: "filename" is needed.
You can query the file name through DebugLoc:
DebugLoc dl;
DIScope *ds = cast<DIScope>(dl.getScope());
ds->getFilename();
as the filename could not be too much. Storing file name per instruction will waste too much memory.
You can store them globally. And use an index to point to the global file-name array.

Thanks!
Ruiling
> -----Original Message-----
> From: Song, Ruiling
> Sent: Tuesday, November 3, 2015 10:32 AM
> To: Bai Yannan; beignet at lists.freedesktop.org
> Cc: Bai, Yannan
> Subject: RE: [Beignet] [PATCH 5/6] Debug Support: Pass debug infomation to
> GenInsn
> 
> >
> > +  extern bool OCL_PROFILING; // first defined by calling BVAR in program.cpp
> > +#define SET_GENINSN_DBGINFO(I) \
> > +  if(OCL_PROFILING) { \
> > +    p->DBGInfo.line = I.DBGInfo.line; \
> > +    p->DBGInfo.col = I.DBGInfo.col;}
> Please don't assign the fields one by one in DBGInfo, please use p->DBGInfo =
> l.DBGInfo.
> I would like to see a unified definition of the DebugInfo class across the whole
> backend. Don't define it here and there.
> In the current implementation, only the line/column are gathered, the "file
> name" is skipped, which is also very important.
> We may add it later. So at that time, we need to modify the so many definition
> of the DebugInfo scattered across the whole backend.
> 
> > diff --git a/backend/src/backend/gen_encoder.cpp
> > b/backend/src/backend/gen_encoder.cpp
> > index 2cc51cc..5eaddf1 100644
> > --- a/backend/src/backend/gen_encoder.cpp
> > +++ b/backend/src/backend/gen_encoder.cpp
> > @@ -591,11 +591,26 @@ namespace gbe
> >        this->setSrc1(insn, bti);
> >      }
> >    }
> > +
> > +  extern bool OCL_PROFILING; // first defined by calling BVAR in program.cpp
> > +  void GenEncoder::setDBGInfo(uint32_t line, uint32_t col, bool hasHigh)
> 
> So you'd better define the function prototype as GenEncoder::setDBGInfo
> (DebugInfo info, bool hasHigh)
> 
> > --- a/backend/src/backend/gen_encoder.hpp
> > +++ b/backend/src/backend/gen_encoder.hpp
> > @@ -88,6 +88,12 @@ namespace gbe
> >      uint32_t deviceID;
> >      /*! simd width for this codegen */
> >      uint32_t simdWidth;
> > +    struct {
> > +      uint32_t line;
> > +      uint32_t col;
> > +    } DBGInfo;
> 
> > +    vector<GenInsnDBGInfo> storedbg;
> Please don't define variables in the .hpp file. Please move it to .cpp file. If the
> header file is included in many source file, it would have multiple definitions.
> 
> Thanks!
> Ruiling
> > +    void setDBGInfo(uint32_t line, uint32_t col, bool hasHigh);
> >      ////////////////////////////////////////////////////////////////////////
> >      // Encoding functions
> >      ////////////////////////////////////////////////////////////////////////
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet