[PATCH] D39556: [ConstantFold] Support vector index when factoring out GEP index into preceding dimensions

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
Comment at: lib/IR/ConstantFold.cpp:2214
+  bool Unknown =
+      !isa<ConstantInt>(Idxs[0]) && (!isa<ConstantDataVector>(Idxs[0]));
   for (unsigned i = 1, e = Idxs.size(); i != e;
----------------
You should be able to just use `!isa<Constant>(Idxs[0])` for these kinds of checks I think. All GEP indices are required to be integers or integer vectors.


================
Comment at: lib/IR/ConstantFold.cpp:2250
+      }
+    } else if (ConstantDataVector *CV = dyn_cast<ConstantDataVector>(Idxs[i])) {
+      bool InRange = true;
----------------
This should be an `else` instead of an `else if`. `Idxs[i]` should be a vector here, so just cast it without checking.


================
Comment at: lib/IR/ConstantFold.cpp:2253
+      for (unsigned I = 0, E = CV->getNumElements(); I != E; ++I) {
+        ConstantInt *CI = cast<ConstantInt>(CV->getElementAsConstant(I));
+        InRange &= isIndexInRangeOfArrayType(STy->getNumElements(), CI);
----------------
Use auto.


================
Comment at: lib/IR/ConstantFold.cpp:2279-2280
+    // if necessary.
+    Constant *CurrIdx = cast<Constant>(Idxs[i]);
+    Constant *PrevIdx =
+        NewIdxs[i - 1] ? NewIdxs[i - 1] : cast<Constant>(Idxs[i - 1]);
----------------
Use auto


================
Comment at: lib/IR/ConstantFold.cpp:2285-2290
+    uint64_t IdxNumElements =
+        IsCurrIdxVector
+            ? cast<SequentialType>(CurrIdx->getType())->getNumElements()
+            : IsPrevIdxVector
+                  ? cast<SequentialType>(PrevIdx->getType())->getNumElements()
+                  : 0;
----------------
This is confusing; I would get rid of it. The zero value is never used either. I would just use `PrevIdx->getType->getNumVectorElements()` or the equivalent for `CurrIdx` whenever you need `IdxNumElements` (see below for an example).


================
Comment at: lib/IR/ConstantFold.cpp:2293
+    if (!IsCurrIdxVector && IsPrevIdxVector)
+      CurrIdx = ConstantDataVector::getSplat(IdxNumElements, CurrIdx);
+
----------------
CurrIdx = ConstantDataVector::getSplat(PrevIdx->getType()->getNumVectorElements(), CurrIdx);


================
Comment at: lib/IR/ConstantFold.cpp:2296
+    if (!IsPrevIdxVector && IsCurrIdxVector)
+      PrevIdx = ConstantDataVector::getSplat(IdxNumElements, PrevIdx);
+
----------------
PrevIdx = ConstantDataVector::getSplat(CurrIdx->getType()->getNumVectorElements(), PrevIdx);


Repository:
  rL LLVM

https://reviews.llvm.org/D39556