[PATCH] D12062: Allow values to cause memory accesses

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

I looked through this patch and saw a couple of the code simplifications you mention in the commit message, but also got the feeling that several changes introduced change behaviour in a way that is somehow confusing to me. Overall, I am not convinced this patch is an improvement as it is.

Best,
Tobias


================
Comment at: include/polly/ScopInfo.h:246-247
@@ -245,4 +245,4 @@
 
-  /// @brief The access instruction of this memory access.
-  Instruction *Inst;
+  /// @brief The access value that caused this memory access.
+  Value *AccessValue;
 
----------------
Similar to Michael, the idea of an "AccessValue" is not understandable to me. The idea of an access instruction was that the access happend at the given instruction. Obviously, a llvm::Value does not have a location and consequently does not trigger a memory access.

You probably have some ideas in mind, but without an explanation of what the AccessValue is meant to be this is unfortunately not understandable to me.

================
Comment at: include/polly/ScopInfo.h:649
@@ +648,3 @@
+  /// @brief Return the (scalar) memory accesses for @p Val.
+  const MemoryAccessList &getAccessesFor(const Value *Val) const {
+    MemoryAccessList *MAL = lookupAccessesFor(Val);
----------------
My feeling is that this function now changes behavior.

Bevor, for a given instruction the memory accesses we model for this instruction were returned.

Now, look at this example:

```
bb1:
  sum = add i64 ...  
  br bb2

bb2:
  br bb3             

bb3
  merge = phi [%sum, %bb2], ...

```

If I now call getAccessFor("%sum"), it will also return the write of %sum into "merge.phiops", even though this memory access is not really performed by the instruction %sum, but it just uses the value %sum.

Besides being unintuitive, I am afraid this change has a risk of breaking other (unrelated parts) of Polly. Specifically, if a value 
loaded by a "load" instruction is used in a PHI node, Stmt.getAccessFor("load") will return only one out of two memory accesses. As the PHI access is likely to be constructed later
and will be inserted at the front, the first access is most likely the wrong one.

```
void VectorBlockGenerator::generateLoad(ScopStmt &Stmt, const LoadInst *Load,    
                                        ValueMapT &VectorMap,                    
                                        VectorValueMapT &ScalarMaps) {           
  if (!VectorType::isValidElementType(Load->getType())) {                        
    for (int i = 0; i < getVectorWidth(); i++)                                   
      ScalarMaps[i][Load] =                                                      
          generateScalarLoad(Stmt, Load, ScalarMaps[i], GlobalMaps[i], VLTS[i]); 
    return;                                                                      
  }                                                                              
                                                                                 
  const MemoryAccess &Access = Stmt.getAccessFor(Load);  
```


================
Comment at: lib/Analysis/ScopInfo.cpp:974
@@ -978,1 +973,3 @@
+    if (StoreMA->isScalar())
+      continue;
     if (StoreMA->isRead())
----------------
This change is also confusing. Does this mean we will not support scalar reductions any more?


http://reviews.llvm.org/D12062