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

[Hongbin Zheng via llvm-commits]

etherzhhb added a comment.

Will refine the patch


================
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>
----------------
Meinersbur wrote:
> 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.
ok

================
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);
   }
----------------
Meinersbur wrote:
> 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.
It works if we introduce simplify_type, and I actually experimented this and confirm it work.


I think it should be
```
return I && (llvm::isa<llvm::LoadInst>(I) || llvm::isa<llvm::StoreInst>(I));
```

================
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;
----------------
Meinersbur wrote:
> 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.
ok

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

================
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();
   }
----------------
Meinersbur wrote:
> Is there any advantage of this scheme that two ifs? Both have 5 lines.
I dont think it has advantage, this together with the following macro are copied from CallSite... I first copy here then copy to the following marco

================
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());
   }
----------------
Meinersbur wrote:
> I'd rather avoid using macros, but still OK.
We may think about using template to do the same thing when we have time.


Repository:
  rL LLVM

http://reviews.llvm.org/D17250