[PATCH] D136102: [LoopSimplify] Update loop-metadata ID after loop-simplify splitting out new outer loop

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopSimplify.cpp:336
 
+  // Create new metadata ID for NewOuter loop
+  // keep other metadata attched before
----------------
Narutoworld wrote:
> bjope wrote:
> > I'm no expert on the Loop metadata handling. But in some sense I'd consider the outer loop as the original loop (so keeping the old metadata on that loop seem correct, e.g. if it has been vectorizer, or if user has forced vectorization of that original loop etc).
> > 
> > However, I'm not sure if the metadata should be kept for the inner loop here. That is a sub-loop, and not sure what an optimization report should say about that loop (that was created by the compiler). And if the user has requested unrolling of the original loop with a factor X, and this loop simplify pass is run before unrolling, we now indicate that we want both the outer and inner loop to be unrolled by a factor X. That seem wrong to me. So maybe it would be more correct to just drop all metadata for the inner loop (except for the loop id).
> > 
> > On the other hand, hypothetically, if the metadata already indicates that the loop has been vectorized/unrolled etc, then we probably want to mark the inner loop as already having been optimized to avoid that it is optimized again. Or is that irrelevant here?
> Thanks for your comment.
> 
> For the given testcase, the outer loop needs the `llvm.loop.usefulinfo` metadata not the inner loop, which happens to have it only because they were a single loop. 
> 
> However, I don't think we can generalize this. And in fact, the `loop-simplify` pass tries to create the outer loop and keeps the remaining loop as before. It might be better to drop all metadata if we cannot prove they are valid. 
> 
> I think it should be the vectorizer's job to mark a loop that has already been optimized . 
> I think it should be the vectorizer's job to mark a loop that has already been optimized .

That is true. But the question is how a pass like this should behave if in the input
a) a loop already is marked as having been vectorized/unrolled etc
b) a loop is marked as "force vectorize" or "force unroll X times" etc

In case (a) I think the metadata probably should be kept on the outer loop (and possibly added to the inner loops) The optimization has already been done and we do not want further vectorization/unrolling on any of the sub loops.

In case (b) I think the metadata probably should remain on the outer loop (which in this case is the full original loop?). But it should not be added to the inner loop (or else we might get unrolling by a factor X*X etc).

Anyway. I believe it would be correct to keep the original metadata on the outer loop.
Not quite sure about which metadata to put on the inner loop. Dropping all metadata (except the loop id) for the inner loop is perhaps good enough for this patch to just sort out the current issue that we get same loop id on both inner and outer loop, and I think adding the outer loops metadata to the inner loop could be worse than dropping the metadata on the inner loop.

So in other words, I think the patch should be updated to get this kind of semantics:
- replicate the original loops metadata to the new outer loop
- assign a new loop id to the inner loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136102