[PATCH] improve scheduling in SLPVectorizer

[Eric Christopher]

Some style comments:

+  struct ScheduleData {
+
+    ScheduleData() :
+          Inst(nullptr), SchedulingPriority(0), Dependencies(-1),
+          UnscheduledDeps(-1) {
+      // Not really required in the constructor (except SchedulingRegion) as
+      // it's done as soon as it becomes a member of the scheduling region.
+      init(-1);
+    }

-  /// \brief Get the corresponding instruction numbering list for a given
-  /// BasicBlock. The list is allocated lazily.
-  BlockNumbering &getBlockNumbering(BasicBlock *BB) {
-    auto I = BlocksNumbers.insert(std::make_pair(BB, BlockNumbering(BB)));
-    return I.first->second;
-  }
+    void init(int BlockSchedulingRegionID) {
+      FirstInBundle = this;
+      NextInBundle = nullptr;
+      NextLoadStore = nullptr;
+      IsScheduled = false;
+      SchedulingRegionID = BlockSchedulingRegionID;
+      UnscheduledDepsInBundle = UnscheduledDeps;
+      clearDependencies();
+    }


Please go ahead and initialize everything in the constructor without
the function call if possible. (Same for BlockScheduling).


+    BS = new BlockScheduling(BB);
+    BlocksSchedules[BB] = std::unique_ptr<BlockScheduling>(BS);

std::make_unique?

It'd also be nice if you could break this up to make it a bit more
incremental. I.e. things like the statistic can just be added first.

+bool BoUpSLP::BlockScheduling::tryScheduleBundle(ArrayRef<Value *> VL,
+                                                 AliasAnalysis *AA) {

Block comment above this function documenting it?

+            // Should never happen, just to be save.

"safe"

You're also using 1/-1 a lot as constant arguments to functions. Is
there a way you can either make this an enum or constant propagate it
for fewer magic constants?

Thanks.

-eric

On Tue, Jul 15, 2014 at 1:37 AM, Erik Eckstein <eeckstein at apple.com> wrote:
> Hi,
>
> This patch improves the scheduling in the SLPVectorizer. Originally the
> SLPVectorizer could only move instructions of a bundle down. The new
> scheduling algorithm is more general, which means that (in theory) it should
> always schedule better than the old approach.
>
> The change fixes: <rdar://problem/13677020> [SLP vectorizer] - SLP
> Vectorizer needs to be able to start trees at multiple user reductions.
> Another example is Bug 19657, where the new algorithm replaces the need for
> the heuristics for fixing it.
>
> The compile time and execution times for the test suite look ok. Measured
> with the test suite on x86.
>
> Compile time: approximately the same as before. The new algorithm fixes a
> compile time problem for the (synthetic) test
> SingleSource/UnitTest/Vector/constpool. This compiles much faster now.
>
> Execution time: not too much changes. A few benchmarks show improvements
> when compiled with -O3 -flto.
>
> Statistics: With the new scheduling algorithm the SLPVectorizer can generate
> about 9% more llvm vector instructions. Note that this is a static count,
> not the number of _executed_ instructions (So if not inside a critical loop
> it will not show up in execution time).
>
> As this patch is not trivial, I plan to commit it after the 3.5 branch.
>
> Please let me know your comments (Arnold and Nadav already looked at the
> patch).
>
> Thanks,
> Erik
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>