[llvm-commits] [PATCH] Configurable CallSiteBase

[Dan Gohman]

On Mar 30, 2010, at 2:34 AM, Gabor Greif wrote:

> Hi all,
> 
> I'd like to put this patch to review. It pulls out some
> functionality from CallSite into a template baseclass
> CallSiteBase. CallSiteBase is highly customizable in the
> type of the inputs and outputs (types), especially constness
> variants. CallSite then chooses the fully non-const configuration.

Hi Gabor, this looks interesting. I have a few comments.

> CallSiteBase<> with all-default parameters implements the fully
> const configuration. This is the preferred configuration for
> analyses and predicates, because it prevents accidental mutation.

Did you find any accidental mutations?

> It is not yet settled how I should arrange the order of
> CallSiteBase template parameters. Logically, those that are
> frequently customized should go to the front, but I have no
> data points yet, since all we use now is an all-or-nothing
> regarding constness.

I don't think there aren't any interesting customizations
besides flipping constness.

> 
> Some predicates of analyses already have been converted to
> full-constness to demonstrate the concept. More to follow.
> 
> There is potential for pulling more methods into the base class,
> I plan to do this as soon as need arises.
> 
> Notable omissions:
> - ProgrammersManual: document CallSiteBase
> - should we define a class ConstCallSite? (I see no gain ATM.)

Yes -- a class, or perhaps a typedef. 7-ary templates are fearsome, even
when fully defaulted. Since there's little need for general customization,
presenting the two main use cases as regular types makes it easier to
understand the API.

> 
> CallSiteBase now provides three convenience features (some of
> them are inherited by CallSite too) :
> - operator bool: this conversion allows to check whether we have
>   a proper call site. Allows to eliminate an indentation level of
>   "if" statements.
> - operator ->: gives back the payload as an instruction (InstrTy*)
> - constructor from ValueTy: this may supplant the static "get"
>   method in the future (ATM it is implementer in terms of "get").
>   Anyway, this constructor can be used in "if" statements to
>   get rid of a free-standing wide-scope local binding.

Another way to provide this functionality would be to redo the CallSite
abstraction as a pseudo-class, following the IntrinsicInst.h example.
That way, it could use dyn_cast like everything else instead of its
own syntax.

For example, something like this:

class CallSite : public User {
   CallSite();                       // DO NOT IMPLEMENT
   CallSite(const CallSite &);       // DO NOT IMPLEMENT
   void operator=(const CallSite &); // DO NOT IMPLEMENT
public:
    // ...
    
    // Methods for support type inquiry through isa, cast, and dyn_cast:
    static inline bool classof(const CallSite *) { return true; }
    static inline bool classof(const CallInst *I) { return true; }
    static inline bool classof(const InvokeInst *I) { return true; }
    static inline bool classof(const Value *V) {
      return isa<CallInst>(V) || isa<InvokeInst>(V);
    }
};

I don't have a strong opinion about this though.

Dan