[PATCH] D22867: [LV] Untangle the concepts of uniform and scalar

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6356-6360
@@ -6263,40 +6355,7 @@
   // Insert uniform instruction into VecValuesToIgnore.
-  // Collect non-gather/scatter and non-consecutive ptr in NonConsecutivePtr.
-  SmallPtrSet<Instruction *, 8> NonConsecutivePtr;
-  for (auto *BB : TheLoop->getBlocks()) {
-    for (auto &I : *BB) {
+  for (auto *BB : TheLoop->getBlocks())
+    for (auto &I : *BB)
       if (Legal->isUniformAfterVectorization(&I))
         VecValuesToIgnore.insert(&I);
-      Instruction *PI = dyn_cast_or_null<Instruction>(getPointerOperand(&I));
-      if (PI && !Legal->isConsecutivePtr(PI) &&
-          !Legal->isLegalGatherOrScatter(&I))
-        NonConsecutivePtr.insert(PI);
-    }
-  }
-
-  // Ignore induction phis that are either used in uniform instructions or
-  // NonConsecutivePtr.
-  for (auto &Induction : *Legal->getInductionVars()) {
-    auto *PN = Induction.first;
-    auto *UpdateV = PN->getIncomingValueForBlock(TheLoop->getLoopLatch());
-
-    if (std::all_of(PN->user_begin(), PN->user_end(),
-                    [&](User *U) -> bool {
-                      Instruction *UI = dyn_cast<Instruction>(U);
-                      return U == UpdateV || !TheLoop->contains(UI) ||
-                             Legal->isUniformAfterVectorization(UI) ||
-                             NonConsecutivePtr.count(UI);
-                    }) &&
-        std::all_of(UpdateV->user_begin(), UpdateV->user_end(),
-                    [&](User *U) -> bool {
-                      Instruction *UI = dyn_cast<Instruction>(U);
-                      return U == PN || !TheLoop->contains(UI) ||
-                             Legal->isUniformAfterVectorization(UI) ||
-                             NonConsecutivePtr.count(UI);
-                    })) {
-      VecValuesToIgnore.insert(PN);
-      VecValuesToIgnore.insert(UpdateV);
-    }
-  }
 }
 
----------------
mssimpso wrote:
> anemet wrote:
> > This is essentially LGTM now (assuming that @wmi an @mkuper are happy too) and I am only wondering about test-coverage now and that the patch really only changes the described functionality.
> > 
> > 1. Do we already have test-coverage for this in the original version?
> > 
> > 2. Did you try to perf-test this patch or in combination with some other patches?  The new version is way better and we can actually reason about what's going on (thank you!) but want to make sure that we didn't miss anything.
> > 
> > Thanks for your work! 
> Thanks Adam!
> 
> > Do we already have test-coverage for this in the original version?
> 
> We have the existing induction variable scalarization tests, which all continue to pass. Previously, we relied on the IVs being in VecValuesToIgnore. Beyond that we have at least one register usage test (X86), which continues to pass. calculateRegisterUsage also relies on VecValuesToIgnore.
> 
> > Did you try to perf-test this patch or in combination with some other patches?
> 
> Yes, I tested this patch together with D22869 on spec2006 and spec2000 (LTO, Kryo). I saw small improvements in several benchmarks with no non-noise regressions. The most significant improvements were in spec2006/h264ref and spec2000/mesa and were on the order of 1-2%.
> 
I should have also pointed out to everyone for further clarity that the code I removed here was already (mostly) duplicated. Over in collectLoopUniforms, we mark IVs uniform if they or their updates are only used by uniforms. Thus, here in collectValuesToIgnore, the uniform IVs would have already been added to VecValuesToIgnore (line 6269).

The functional change here is that now we are not adding to VecValuesToIgnore the non-uniform IVs that are used by non-consecutive pointers. This makes sense to me, as I mentioned in my reply to Wei, since we will still create a scalar value for such IVs corresponding to every iteration of the original scalar loop.


https://reviews.llvm.org/D22867