[PATCH] D23169: [LV] Unify vector and scalar maps

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Sorry for the delay, thanks for nudging me!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:558
@@ +557,3 @@
+    /// scalar value is initialized to this value.
+    ScalarParts &initScalar(Value *Key, Value *Val = nullptr) {
+      assert(!hasScalar(Key) && "ScalarParts already initialized");
----------------
Do we need the Val parameter? It doesn't look like we call this with a non-default Val anywhere.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2322
@@ +2321,3 @@
+    // If V doesn't produce a value, just return the initialized entry.
+    if (V->getType()->isVoidTy())
+      return Entry;
----------------
This looks a bit odd.

When does this happen? I don't think we call getVectorValue() for stores - and if we do, I'm not sure what the expected result is.
If it doesn't happen, perhaps this ought to be an assert?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2368
@@ +2367,3 @@
+  auto *U = getVectorValue(V)[Part];
+  if (!U->getType()->isVectorTy())
+    return U;
----------------
I'd still appreciate a comment (and maybe an assert) that this is for VF==1.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2371
@@ +2370,3 @@
+
+  // Otherwise, the value from the original loop has been vectorized and is
+  // represented by UF vector values. Extract and return the requested scalar
----------------
The comment is a bit odd, because the "Otherwise" looks like it refers to the previous comment ("If the value has not been scalarized...") while in fact it refers to the one before ("If the value from the original loop has not been vectorized").

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2861
@@ +2860,3 @@
+  // VectorLoopValueMap and get a new scalar entry instead.
+  VectorLoopValueMap.eraseVector(Instr);
+  auto &Entry = VectorLoopValueMap.initScalar(Instr);
----------------
Just to make sure I'm not missing anything - the reason this is needed is because we eagerly created the vector entry in the beginning of vectorizeBlockInLoop(), right? We don't *actually* have a vector entry at this point, it's just an empty placeholder - but we need to delete it, so that if we need a real vector entry, it will get constructed from scratch from the scalars?


https://reviews.llvm.org/D23169