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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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:
> 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
“LoopID” is a misnomer, the same LoopID can refer to the same loop. Otherwise every transformation that clones code (unroll, inline, LoopVersioning, LoopUnswitch, ...) whout have to specifically handle any llvm.loop that the cloned code contains. See [[ https://llvm.org/docs/LangRef.html#llvm-loop | LLVM Language Reference Manual ]]. 

I think the default behavior is just applying the same LoopID to both loops such as analytical information is preserved (e.g. `isvectorized`, `parallel_accesses`, ...). It may be argued this is not safe for all metadata since neither loops is quite the original and need to have a list of known loop metadata with the info whether is should apply to the inner loop (`llvm.loop.unroll.enable`, ..), the outer loop (?, can't think of one where this makes most sense), or both loops (`isvectorized`, `parallel_accesses`, `llvm.loop.licm.disable`, all the disable metadata, ...). and discard all unknown.

Some metadata definitely have different semantics when applied the outer loop (only). For instance, when (partial) unrolling an outer loop, it would give multiple copies of the inner loop (which makes the code more inefficient as it would most likely exceed the L1i cache), but not jam like `llvm.loop.unroll_and_jam` might do. `llvm.loop.unroll.disable` would still allow unrolling the inner loop, which is what the heuristic would most likely unroll.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136102