[PATCH] Global Structure Vectorization

[Arnold Schwaighofer]

This looks like unnecessary code duplication:

+      AliasAnalysis::Location ThisLoc = AA->getLocation(Load);
+      std::pair<StoreAliasMap::iterator, StoreAliasMap::iterator> range =
+          WriteObjects.equal_range(*UI);
+      for (StoreAliasMap::iterator it=range.first, end=range.second;
+            it != end; ++it) {
+        StoreInst* S = (*it).second;
+        DEBUG(dbgs() << "LV: --> Against store:" << *S <<"\n");
+
+        AliasAnalysis::Location ThatLoc = AA->getLocation(S);
+        if (AA->alias(ThisLoc.getWithNewSize(MaxByteWidth),
+                      ThatLoc.getWithNewSize(MaxByteWidth))) {
+          DEBUG(dbgs() << "LV: Found a possible write-write reorder:"
+                << *Val <<"\n");
+          return false;


+      // If global alias, make sure they do alias
+      AliasAnalysis::Location ThisLoc = AA->getLocation(Store);
+      std::pair<StoreAliasMap::iterator, StoreAliasMap::iterator> range =
+          WriteObjects.equal_range(*UI);
+      for (StoreAliasMap::iterator it=range.first, end=range.second;
+            it != end; ++it) {
+        StoreInst* S = (*it).second;
+        if (S == Store)
+          continue;
+        DEBUG(dbgs() << "LV: --> Against store:" << *S <<"\n");
+
+        AliasAnalysis::Location ThatLoc = AA->getLocation(S);
+        if (AA->alias(ThisLoc.getWithNewSize(MaxByteWidth),
+                      ThatLoc.getWithNewSize(MaxByteWidth))) {
+          DEBUG(dbgs() << "LV: Found a possible write-write reorder:"
+                << *Val <<"\n");
+          return false;
+        }
+      }

Can you put this into a function?

Can you move the test cases into one file? The more single file .ll tests we have the more we pay for file open/close operations. And we want make check to stay fast :).

On Feb 20, 2013, at 6:48 AM, Renato Golin <renato.golin at linaro.org> wrote:

> Hi folks,
> 
> I haven't changed the patch much since last review for two reasons:
> 
> 1. Using DenseMap<vector> would over-complicate the code, at least for this first introduction. Since Hal only found 1% of improvement with that change, I think we can leave it for later, when the code is more stable.
> 


Why do you say that?

Assuming something like DenseMap<Value*, SmallVector<Instruction*, 4>> or DenseMap<Value*, std::vector<Instruction*, 4>>

You would use a DenseMap<..>::iterator where you use the StoreAliasMap::iterator .

Instead of "std::pair<StoreAliasMap::iterator, StoreAliasMap::iterator> range =  WriteObjects.equal_range(*UI)" you would iterate over the returned SmallVector.

Instead of "Reads.insert(std::make_pair(Ptr, LD))" you would write Reads[Ptr].push_back(LD).

Am I missing something?


> 2. I haven't merged <GEP, UndObj, Store/Load> into one container or structure because it'd again over-complicate the code and it's possible that it'll change as we progress in the alias analysis side of things. I'd rather leave that change for a time when we're sure it won't change much.
> 
> What I have changed:
> 
> 1. Accounting for unroll factor, as Arnold suggested, for the access width.
> 
> 2. Changed from WriteObjects from [!count(*UI)] to [find(*UI) == end()], because we don't need to know how many (thus find all occurrences), just to know that there isn't any (find any occurrence).
> 
> 3. Added some more comments and TODOs to make it clear that this is work in progress.
> 
> Shall we consider this the first step (of many) towards global structure vectorization?
> 

I think so.


Nitpicking:

+  /// Target Info
+  TargetTransformInfo *TTI;
+  /// Alias Analysis

Punctuation.

+  // Biggest vectorized access possible, vector width * unroll factor
+  // TODO: We're being very pessimistic here, find a way to know the
+  // real access width before getting here
+  unsigned MaxByteWidth = (TTI->getRegisterBitWidth(true) / 8) *
+                           TTI->getMaximumUnrollFactor();
   // Now that the pointers are in two lists (Reads and ReadWrites), we
   // can check that there are no conflicts between each of the writes and
   // between the writes to the reads.
-  ValueSet WriteObjects;
+  // Note that WriteObjects duplicates the stores (indexed now by underlying
+  // objects) to avoid pointing to elements inside ReadWrites
+  // TODO: Maybe create a new type where they can interact without duplication
+  StoreAliasMap WriteObjects;


Punctuation.


+      // Never seen it before, can't alias
+      if (WriteObjects.find(*UI) == WriteObjects.end()) {


+      // Direct alias found
+      if (!AA || dyn_cast<GlobalValue>(*UI) == NULL) {
 

+      // If global alias, make sure they do alias
+      AliasAnalysis::Location ThisLoc = AA->getLocation(Store);

+      // Didn't alias, insert into map for further reference
+      WriteObjects.insert(std::make_pair(*UI, Store));

Punctuation.

Ditto for the load cases.


Thanks,
Arnold