This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Introduce MemAccInst helper class; NFC
ClosedPublic

Authored by Meinersbur on Jan 25 2016, 5:16 AM.

Details

Summary

MemAccInst wraps the common members of LoadInst and StoreInst. Add uses of this class to

  • ScopInfo::buildMemoryAccess
  • BlockGenerator::generateLocationAccessed
  • ScopInfo::addArrayAccess
  • Scop::buildAliasGroups
  • wherever polly::getPointerOperand was used

The class has been extracted out of D12975 (DeLICM) where it is used more extensively.

Diff Detail

Event Timeline

Meinersbur updated this revision to Diff 45857.Jan 25 2016, 5:16 AM
Meinersbur retitled this revision from to [Polly] Introduce MemAccInst helper class; NFC.
Meinersbur updated this object.
Meinersbur added reviewers: grosser, jdoerfert.
Meinersbur added a project: Restricted Project.
Meinersbur added subscribers: pollydev, llvm-commits.

+/ getValueOperand() and getPointerOperand() have different implementations in
+
/ LoadInst and StoreInst, but for most other members the implementation is
+/ identical. Those have been reproduced here to avoid useless branching to
+
/ identical implementations. If those implementations change in
+/// LoadInst/StoreInst, they must be changed here as well.

The last sentence alone guarantees problems. Duplication is (almost)
never a solution, especially not to save a call/jump/branch.

+/ getValueOperand() and getPointerOperand() have different implementations in
+
/ LoadInst and StoreInst, but for most other members the implementation is
+/ identical. Those have been reproduced here to avoid useless branching to
+
/ identical implementations. If those implementations change in
+/// LoadInst/StoreInst, they must be changed here as well.

The last sentence alone guarantees problems. Duplication is (almost)
never a solution, especially not to save a call/jump/branch.

We'd notice immediately, but I can change to calling the proper functions, no problem.

Note that this is also duplicated in LLVM itself.

Meinersbur updated this revision to Diff 45865.Jan 25 2016, 7:24 AM

Do not duplicate code from LLVM.

grosser edited edge metadata.Jan 26 2016, 3:32 AM

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?

Tobias

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.

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.

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

One more use could be InstructionToAccess mapping, because since D13676 we only map loads/stores.

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

Meinersbur updated this revision to Diff 45999.Jan 26 2016, 8:37 AM
Meinersbur edited edge metadata.

Proxy object variant (like CallSite, StringRef, ArrayRef)

Hi Michael,

this looks a lot better. Johannes pointed out to me that there is already a function polly::getPointerOperand in ScopHelper.cpp. Having both is probably redundant. I think the GetElementPtrInst is never used in this function (please check). So we can probably eliminate it and just use your class instead of the getPointerOperand instruction. Otherwise, this is probably good to go.

Meinersbur updated this revision to Diff 46136.Jan 27 2016, 7:35 AM
Meinersbur updated this object.

Replace polly::getPointerOperand(); Move class MemAccInst to ScopHelper.h to make it available in ScopDetection.h

grosser accepted this revision.Jan 27 2016, 7:41 AM
grosser edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jan 27 2016, 7:41 AM
This revision was automatically updated to reflect the committed changes.