[PATCH] merge consecutive stores of extracted vector elements (PR21711)

[Quentin Colombet]

Hi Sanjay,

LGTM with some nitpicks.

No need for another round of review.

Thanks,
-Quentin


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:384
@@ +383,3 @@
+    /// \return True if a merged store was created.
+    bool MergeStoresOfConstantsOrVecElts(SmallVector<MemOpLink, 8> &StoreNodes,
+                                         EVT MemVT, unsigned NumElem,
----------------
Use SmallVectorImpl<MemOpLink> instead.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9738
@@ +9737,3 @@
+  SDValue StoredVal;
+  if (!IsConstantSrc) {
+    // Merging vector elements implies using a vector store.
----------------
I would do one more refactoring here.
I would just check for UseVector and having an assert that if IsConstantSrc is true then UseVector must be true as well.

That way instead of having:
if (!IsConstantSrc) {
   // look for type.
   // loop.
   // set res val
} else if (UseVector) {
   // look for type.
   // set res val.
} else {
}

We would have:
if (UseVector) {
  // look for type.
  if (!IsConstantSrc) {
    // loop.
    // set val.
  } else {
   // set val.
  }
} else {
}

The reasons why I suggest this (I am fine if you do not want to do that), are:
1. The type creation will not be duplicated as well as the assert.
2. At first glanced I found confusing to have !IsConstantSrc and UseVector ending in different blocks. Indeed, per the comment, if it is not a constant, this is a vector so having a vector not using vector is confusing.
The alternative would be to add a comment on the actual usage of UseVector in the function prototype.

http://reviews.llvm.org/D6850

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/