This is an archive of the discontinued LLVM Phabricator instance.

[Polly] [MemAccInst] Delete all the assignment operators, as they are not need.
AcceptedPublic

Authored by etherzhhb on Feb 26 2016, 3:09 AM.

Details

Summary

Allow changing the underlying instruction, especially changing the MemAccInst from non-null to null is very confusing and error-prone. But the good news, we never did that. So remove these unused operator to send a clear message that: Changing the underlying instruction is not encouraged.

Diff Detail

Event Timeline

etherzhhb updated this revision to Diff 49170.Feb 26 2016, 3:09 AM
etherzhhb retitled this revision from to [Polly] [MemAccInst] Delete all the assignment operators, as they are not need..
etherzhhb updated this object.
etherzhhb added a reviewer: Meinersbur.
etherzhhb set the repository for this revision to rL LLVM.
etherzhhb added a project: Restricted Project.
etherzhhb added a subscriber: Restricted Project.
etherzhhb added a subscriber: jdoerfert.

Maybe @jdoerfert has some local patches that need the assignment operator?

Dropping dead code is clearly a good thing and this indeed seems like a misuse of MemAccInst. We occasionally allow dead code in the main repository to reduce merge problems with external repositories, but this is clearly an exception. Let's wait for the OK from Johannes and Michael, just in case.

Dropping dead code is clearly a good thing and this indeed seems like a misuse of MemAccInst. We occasionally allow dead code in the main repository to reduce merge problems with external repositories, but this is clearly an exception. Let's wait for the OK from Johannes and Michael, just in case.

yes

etherzhhb removed a subscriber: jdoerfert.
jdoerfert accepted this revision.Feb 26 2016, 6:24 AM
jdoerfert edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 26 2016, 6:24 AM
Meinersbur edited edge metadata.Feb 26 2016, 8:33 AM

I created these for the following reason:

As a general rule for 'pass-by-value' types, for every implicit copy-constructor, there should also also an operator=() for it. Otherwise, this works:

StoreInst *SI = [...]
MemAccInst x = SI;

but not this:

StoreInst *SI = [...]
MemAccInst x;
x = SI;

If assigning a nullptr to a MemAccInst is confusing, then assigning nullptr to a StoreInst* (which it mimics) must be equally confusing.

The only reason I can think of to remove these is that we currently do not assign to a MemAccInst variable.

include/polly/Support/ScopHelper.h
68

The constness of the Instruction pointed to should be determined by the user of MemAccInst by declaring it const, e.g. const MemAccInst.

I created these for the following reason:

As a general rule for 'pass-by-value' types, for every implicit copy-constructor, there should also also an operator=() for it. Otherwise, this works:

StoreInst *SI = [...]
MemAccInst x = SI;

but not this:

StoreInst *SI = [...]
MemAccInst x;
x = SI;

But we did write code like this (yet), right?

If assigning a nullptr to a MemAccInst is confusing, then assigning nullptr to a StoreInst* (which it mimics) must be equally confusing.

Ok.
Allow changing a MemAccInst from load to store also confuse me a little bit.

The only reason I can think of to remove these is that we currently do not assign to a MemAccInst variable.

include/polly/Support/ScopHelper.h
68

This make sense. Could we use MemAccInst just like "const MemAccInst"?

Meinersbur added inline comments.Feb 26 2016, 9:02 AM
include/polly/Support/ScopHelper.h
68

I tired to make MemAccInst mimic a (non-existing) pointer to a base class of LostInst/StoreInst. D17250 makes it even more so. As such I try to keep MemAccInst behaving like a (smart) pointer which includes reassignment and declaring it as const when necessary.

Smart pointers a well-known idiom in that most C++ programmers know. I'd argue differing from a known idiom would be more confusing than changing the pointee.

etherzhhb marked 3 inline comments as done.Feb 26 2016, 9:08 AM
etherzhhb added inline comments.
include/polly/Support/ScopHelper.h
68

Ok

etherzhhb marked an inline comment as done.Feb 26 2016, 5:40 PM

I think I address the wrong question. The right question is:

Should we make all MemAccInst become "const MemAccInst" to guarantee that we do not change the underlying instruction once it is built?

OK, I looked something up: The compiler generates operator= based on the implicit ctors. Declaring them explicitly therefore is unnecessary.

But if I is declared const, the implicit operator=(...) is = deleted.

Could you therefore update the patch that it deletes the overloaded operators, but keeps I non-const?