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

[etherzhhb]

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.

[etherzhhb]

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

[grosser]

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.

[etherzhhb]

In D17641#362658, @grosser wrote:

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

[jdoerfert]

LGTM

[Meinersbur]

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.

[etherzhhb]

In D17641#362857, @Meinersbur wrote:

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]

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]

include/polly/Support/ScopHelper.h

68

Ok

[etherzhhb]

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?

[Meinersbur]

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?

include/polly/Support/ScopHelper.h

 ///
 /// The functions isa, cast, cast_or_null, dyn_cast are modeled te resemble
 /// those from llvm/Support/Casting.h. Partial template function specialization
 /// is currently not supported in C++ such that those cannot be used directly.
 /// (llvm::isa could, but then llvm:cast etc. would not have the expected
 /// behaviour)
 class MemAccInst {
 private:
-  llvm::Instruction *I;
+  llvm::Instruction *const I;
 
 public:
   MemAccInst() : I(nullptr) {}
   MemAccInst(const MemAccInst &Inst) : I(Inst.I) {}
   /* implicit */ MemAccInst(llvm::LoadInst &LI) : I(&LI) {}
   /* implicit */ MemAccInst(llvm::LoadInst *LI) : I(LI) {}
   /* implicit */ MemAccInst(llvm::StoreInst &SI) : I(&SI) {}
   /* implicit */ MemAccInst(llvm::StoreInst *SI) : I(SI) {}
public:
   }
   static MemAccInst dyn_cast(llvm::Value *V) {
     assert(V);
     if (isa(V))
       return MemAccInst(llvm::cast<llvm::Instruction>(V));
     return MemAccInst();
   }
 
-  MemAccInst &operator=(const MemAccInst &Inst) {
-    I = Inst.I;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::LoadInst &LI) {
-    I = &LI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::LoadInst *LI) {
-    I = LI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::StoreInst &SI) {
-    I = &SI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::StoreInst *SI) {
-    I = SI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::MemIntrinsic &MI) {
-    I = &MI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::MemIntrinsic *MI) {
-    I = MI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::CallInst &CI) {
-    I = &CI;
-    return *this;
-  }
-  MemAccInst &operator=(llvm::CallInst *CI) {
-    I = CI;
-    return *this;
-  }
-
   llvm::Instruction *get() const {
     assert(I && "Unexpected nullptr!");
     return I;
   }
   operator llvm::Instruction *() const { return asInstruction(); }
   llvm::Instruction *operator->() const { return get(); }
 
   explicit operator bool() const { return isInstruction(); }