[PATCH] D122148: [SLP] Peak into loads when hitting the RecursionMaxDepth

Thu Mar 24 03:30:45 PDT 2022

dmgreen added a comment.

In D122148#3401136 <https://reviews.llvm.org/D122148#3401136>, @vporpo wrote:

> Thank you for taking the time to measure the compilation time with an increased max-depth limit.
>
> I am not sure what the max-depth limit was designed to be used for, probably for testing gathers in lit test? But I don't think that it is a very good way of limiting compilation time. A better option would be to limit the total number of nodes in the graph, by simply counting the entries in `buildTree_rec()`. So I would assume that increasing the max-depth limit should be relatively safe. If we run into compile-time issues, we can introduce the `buildTree_rec()` entry counter. @ABataev @RKSimon any thoughts on this?

I think I would still recommend this over increasing the max-depth limit. It is more targeted, and less likely to run into degenerate cases where the compile time does increase.  It just increases the limit where we know it will be most valuable. They are not mutually exclusive of course - we can always increase the limit separately if we need to.

================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3998
+  if (Depth >= RecursionMaxDepth &&
+      !(S.MainOp && match(S.MainOp, m_ZExtOrSExtOrSelf(m_Load(m_Value()))))) {
     LLVM_DEBUG(dbgs() << "SLP: Gathering due to max recursion depth.\n");
----------------
ABataev wrote:
> It might be not profitable, e.g. if vector extension is not free, while scalar extension is free, and loads are not vectorizable. Also, what if you have an extension of a load and some other instructions, not loads/extractelements, etc.?
Im not sure I understand what you mean by unprofitable? This just stops zext(load being forced to be gather if it hits the max depth. It should just mean that those node are either better (not gathers) or the same and shouldn't lead to regressions. It was previously hitting an arbitrary limit - you could say the same where any arbitrary limit causes arbitrary problems. Giving the loads the ability to order nicely should be a bigger win.

For the second part - do you mean a AltOp? If so then that makes sense, we can add a check for that, making sure it is the same as the MainOp.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122148/new/

https://reviews.llvm.org/D122148