[PATCH] D79799: [VectorCombine] add loop to enable iterative folding

Sat May 16 08:57:48 PDT 2020

nikic added a comment.

In D79799#2039039 <https://reviews.llvm.org/D79799#2039039>, @spatel wrote:

> In D79799#2038887 <https://reviews.llvm.org/D79799#2038887>, @nikic wrote:
>
> > My only concern here would be whether this can degenerate quadratically. Without particular familiarity with this pass, and looking at example `@ins1_ins1_iterate`, it seems like this would work by scalarizing one operation on each iteration. Is that right? If so, that seems potentially problematic, as the pass becomes quadratic for longer instruction chains.
>
>
> Yes, in the worst case it could become quadratic, and you're seeing the test correctly. I think we could undo the reverse basic-block walk in this patch to make that particular case more efficient.

Speaking of which: If I replace this with a forward walk, then the ins1_ins1_iterate test folds, and there are no other test changes in VectorCombine or PhaseOrdering. If there are cases that benefit from the backwards walk, there doesn't seem to be test coverage for them.

> If I'm seeing it correctly, other iterative passes like SimplifyCFG and CodeGenPrepare allow for quadratic-time possibility too. I'm not sure if there's a theoretical way to draw the line on that, or if we just accept that potential risk (assume that anything this pass will ever do is rare, so it can't be too expensive). As I wrote in the description, I think the alternative is to revise things to be more like InstCombine's user-based worklist. We could also put in a cl::opt flag to bail out and/or assert if we go overboard. Any other suggestions?

I agree that this is unlikely to become a problem in practice for this pass, because vector code is uncommon. And you are right that this problem also exists in other passes, though it is probably not always as easy to find actually quadratic cases. Personally I'm fine with your approach here, and we can always pivot to a worklist if necessary. Though per my comment above, it seems like for now just flipping the iteration order might be sufficient.

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

https://reviews.llvm.org/D79799