This is an archive of the discontinued LLVM Phabricator instance.

[PGO] Implement a more robust/readable Writer callback interface
ClosedPublic

Authored by davidxl on Nov 19 2015, 9:35 PM.

Diff Detail

Event Timeline

davidxl updated this revision to Diff 40740.Nov 19 2015, 9:35 PM
davidxl retitled this revision from to [PGO] Implement a more robust/readable Writer callback interface.
davidxl updated this object.
davidxl added a reviewer: silvas.
davidxl added a subscriber: llvm-commits.
silvas edited edge metadata.Nov 20 2015, 7:36 PM

Thanks, this is a lot cleaner!
I've made some stylistic suggestion to help move the unsafe casting to highly-visible locations, which I think reveals a logic bug (2 potential fixes suggested), but with that fixed this LGTM.

(also a tiny comment typo fix)

lib/profile/InstrProfilingBuffer.c
51

I would rename the argument void *WriterCtx and then have a declaration char *Buffer = (char *)WriterCtx; as the first thing in the function.

But with that change, it becomes clearer that bufferWriter does not maintain the correct state across calls (do we have a test that fails? if not it would be nice to add one). I think as far as making this code clear and boring, I would suggest:

struct BufferWriterCtx {
  char *BufPtr;
};

... inside bufferWriter ...
  BufferWriterCtx *Ctx = (BufferWriterCtx *)WriterCtx;
... inside the loop
    Ctx->BufPtr += Length;

You could probably get by using just char ** instead of a BufferWriterCtx struct, but then you would end up with an expression like char **PtrToBufPtr = (char **)WriterCtx; and (*PtrToBufPtr) += Length which is harder to follow.

Another possibility is for the writer to be called exactly once in all cases by changing llvmWriteProfDataImpl to be something like this:

ProfDataIOVec IOVecs[MAX_IOVECS] = {0};
ProfDataIOVec *NextIOVec = &IOVec[0];
appendIOVec(&NextIOVec, (const char *)&Header, sizeof(__llvm_profile_header), 1);
...
if (ValueDataBegin)
  appendIOVec(&NextIOVec, ValueDataBegin, sizeof(char), ValueDataSize);

Writer(IOVec, NextIOVec - &IOVec[0], WriterCtx);
lib/profile/InstrProfilingFile.c
20

I think this is missing "1" in "Return 1 if there is an error". The comment in bufferWriter is similarly affected.

22

I would rename the argument void *WriterCtx and then have a declaration FILE *File = (FILE *)WriterCtx; as the first thing in the function.

This revision was automatically updated to reflect the committed changes.

I addressed the comments and committed the patch. Good observation about
maintaining writer state across calls to the callback -- I ended passing
pointer to writerCtx pointer to the callback so that it can be updated if
needed. It is not ideal, but also not reduce readability of the code.

thanks,

David