[PATCH] D16530: [Polly] Introduce MemAccInst helper class; NFC

[Tobias Grosser via llvm-commits]

> Meinersbur added a comment.
>
> In http://reviews.llvm.org/D16530#335899, @grosser wrote:
>
>> Hi Michael,
>>
>> I think this patch makes a lot of sense. Unfortunately all these casts make it look rather ugly as they introduce more noise than is actually removed. I just looked quickly at the implementation of CallSite and it seems the implementation is slightly different. Specifically, it does not inherit from instruction, but rather creates a proxy class around the CallInst/InvokeInst. Like this it is possible to add default constructors for CallSite(Invoke Ty) and CallSite(CallTy) that allow for automatic implicit type conversion. Could this be a solution that would allow us to remove the need for explicit casts?
>
>
> Unfortunately their type is LoadInst* instead of LoadInst&. C++ does not support overloading operators/implicit conversions of pointer types. I could change their declarations to references and implement these implicit constructors, but this would create a new MemAccInst every time, i.e. create a new instruction because it is derived from Instruction.

I think it would be OK to change the type to LoadInst& and to construct 
new instances of MemAccInst that keep just a pointer to LoadInst/StoreInst.

Now, I am not sure if there is a real need to actually derive from 
Instruction at all. This seems to add overhead and does not seem to be 
used. CallSiteInst works e.g. also without being derived from Instruction.

> The implementation of CallSite is quite different. It is a stack object containing a pointer to the CallInst or InvokeInst (you called it proxy). This allows "it can also create a null initialized CallSiteBase object for something which is NOT a call site". I don't think we need this, we can just test for nullptr. It also does not allow implicit conversions from pointers and users then are required to think about the lifetime of the stack object. It would allow implicit conversions from LoadInst& though.

Right, the last point is what I am after.

> Anyway, it is only one of the uses where I thought it could make sense. We could also just drop the BlockGenerator::generateLocationAccessed part.

If it can not even be used here, this new class does not seem to be very 
useful. Hence, I am trying to figure out if we can address my last concerns.

> One more use could be InstructionToAccess mapping, because since http://reviews.llvm.org/D13676 we only map loads/stores.

Possibly. However, I think the use in generationLocationAccessed would 
be good enough to justify its introduction.

Best,
Tobias