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

[MengXuan Cai via Phabricator via llvm-commits]

Narutoworld added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopSimplify.cpp:336
 
+  // Create new metadata ID for NewOuter loop
+  // keep other metadata attched before
----------------
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 . 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136102