[llvm-commits] [PATCH] Configurable CallSiteBase

[Gabor Greif]

Am 30.03.2010 um 21:44 schrieb 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?

Not so far, but I have only const-ized a bunch of occurrences.
I was able to remove several non-const --> non-const casts
that were necessitated by the CallSite class, however.

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

Yeah. But I am not done yet. Other use-cases may lurk there.

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

Okay, I'll drop "ImmutableCallSite" in the mix.

>
>>
>> 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);
>     }
> };

Hmmm, CallSite is supposed to have the weight of a pointer, and the  
above
is rather heavy, certainly not "value semantics". I do not like the  
idea.

Cheers,

	Gabor

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