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

[Juergen Ributzka]

On Apr 29, 2014, at 12:09 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
> 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.

That sounds good. I see that the memory manager callbacks do the same, so I will add that.

> 
> 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.

I saw a similar naming schema in LLVMContext (e.g. LLVMDiagnosticHandlerTy), so I followed that.
Although in other places in the C API they are also called callbacks, so going with callback sounds better to me too.

> 
> +  /// 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.

Good point. While I am at it I will add the pre-run hook too.

> 
> 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.

I am not sure I follow here you with your reference to context. This is the naming schema we usually follow for classes in the C API.