[PATCH][PM] Add pass run listeners to the pass manager.

[Duncan P. N. Exon Smith]

On 2014-Apr-28, at 11:27, Juergen Ributzka <juergen at apple.com> wrote:

> Thanks Andy. Committed in r207430.
> -Juergen

Not sure whether to reply here or in response to r207430, but since Chandler
resurrected the pre-commit review thread I'm keeping everything in one place.

+/** @see llvm::PassRunListener */
+typedef struct LLVMOpaquePassRunListener *LLVMPassRunListenerRef;
+
+/** @see llvm::LLVMPassRunListener */
+typedef void (*LLVMPassRunListenerHandlerTy)(LLVMContextRef, LLVMPassRef,
+                                             LLVMModuleRef, LLVMValueRef,
+                                             LLVMBasicBlockRef);
+

I think callbacks are *much* more useful if they include a client handle.

I.e.,

    typedef void (*CallbackType)(void *Handle, /* other args */);
    void registerCallback(/* where */, CallbackType Callback, void *Handle);

This way the client can pass an arbitrary context to `registerCallback()`
and get it back on the other side.

If the client doesn't need a handle, they can always pass nullptr and ignore
the argument.

For clarity, this would change the C API to something like:

    typedef void (*LLVMPassRunListenerHandlerTy)(void *Handle,
                                                 LLVMContextRef, LLVMPassRef,
                                                 LLVMModuleRef, LLVMValueRef,
                                                 LLVMBasicBlockRef);
    LLVMPassRunListenerRef LLVMAddPassRunListener(LLVMContextRef,
                                                  LLVMPassRunListenerHandlerTy,
                                                  void *Handle);

I also think the name `LLVMPassRunListenerHandlerTy` is a little strange.
Something like `LLVMPassRunCallbackTy` makes more sense to me.

+  /// Callback function - This functions is invoked whenever a pass has run.
+  virtual void passRun(LLVMContext *, Pass *, Module *, Function *,
+                       BasicBlock *) {}

I don't like this function name at all.  "handlePassHasRun()" is more
demonstrative; maybe you can think of something better.

None of the naming (class names, C API, function names) makes it clear whether
the pass is *about* to run or has *finished* running.  (Your comments indicate
this is after a run, but the naming should as well.)

I think this is especially important in the C API.  We won't be able to change
names, so if we add a new hook for pre-run, we're stuck with something
inconsistent.

I also think `LLVMPassRunListenerHandlerTy` unnecessarily embeds the context
into the type name.  If we add more hooks in the future, the type name won't
make sense.  I prefer `LLVMPassCallbackTy`, or something like it.