[PATCH] D17250: [Polly] [Refactor] Make MemAccInst more concise.

[Michael Kruse via llvm-commits]

Meinersbur added inline comments.

================
Comment at: include/polly/Support/ScopHelper.h:81
@@ +80,3 @@
+  /* implicit */ MemAccInst(T *I)
+      : I((I && llvm::isa<MemAccInst>(I)) ? I : nullptr) {}
+  template <typename T>
----------------
The distinction between implicit an explicit constructors is lost with this. In particular, I'd avoid an arbitrary Instruction to be converted to MemAccInst without a kind of explicit 'cast'. This makes a execution-time error out of it, including if passing someting that is not even an Instruction.

================
Comment at: include/polly/Support/ScopHelper.h:86
@@ -107,1 +85,3 @@
+  static bool classof(const llvm::Value *I) {
+    return llvm::isa<llvm::LoadInst>(I) || llvm::isa<llvm::StoreInst>(I);
   }
----------------
classof is intended for LLVM's object hierarchy and MemAccInst is no part of it. It is not derived from llvm::Value or similar. That means eg.
```
LoadInst *LI;
cast<MemAccInst>(LI)
```
will go horribly wrong.

================
Comment at: include/polly/Support/ScopHelper.h:97
@@ -123,1 +96,3 @@
+  template <typename T> MemAccInst &operator=(T *RHS) {
+    I = StaticAssertLegalT<T>(RHS);
     return *this;
----------------
This scheme doesn't work in general when we want something implicitly converted to the argument. Though, in this case, because nothing derives from LoadInst or StoreInst, there might be no such case. I'd still prefer the previous for readability.

================
Comment at: include/polly/Support/ScopHelper.h:102
@@ -128,1 +101,3 @@
+  template <typename T> MemAccInst &operator=(T &RHS) {
+    return operator=<T>(&RHS);
   }
----------------
No StaticAssertLegalT here?

================
Comment at: include/polly/Support/ScopHelper.h:106
@@ -130,2 +105,3 @@
   operator llvm::Instruction *() const { return asInstruction(); }
+  llvm::Instruction *operator->() const { return asInstruction(); }
   explicit operator bool() const { return isInstruction(); }
----------------
That's want I suggested an planned as well.

================
Comment at: include/polly/Support/ScopHelper.h:115
@@ -140,9 +114,3 @@
 
-  /// @brief Get the debug location of this instruction.
-  ///
-  /// @returns The debug location of this instruction.
-  const llvm::DebugLoc &getDebugLoc() const {
-    if (I)
-      return I->getDebugLoc();
-    llvm_unreachable("Operation not supported on nullptr");
+    return llvm::cast<llvm::StoreInst>(asInstruction())->getValueOperand();
   }
----------------
Is there any advantage of this scheme that two ifs? Both have 5 lines.

================
Comment at: include/polly/Support/ScopHelper.h:126
@@ -157,7 +125,3 @@
   llvm::Value *getPointerOperand() const {
-    if (isLoad())
-      return asLoad()->getPointerOperand();
-    if (isStore())
-      return asStore()->getPointerOperand();
-    llvm_unreachable("Operation not supported on nullptr");
+    MEMACCINST_DELEGATE_GETTER(getPointerOperand());
   }
----------------
I'd rather avoid using macros, but still OK.

================
Comment at: include/polly/Support/ScopHelper.h:147
@@ +146,3 @@
+///        from a MemAccInst object.
+template <> struct simplify_type<polly::MemAccInst> {
+  typedef Instruction *SimpleType;
----------------
Wow, I didn't know this is possible.


Repository:
  rL LLVM

http://reviews.llvm.org/D17250