[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
----------------
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?


================
Comment at: llvm/test/Transforms/LoopSimplify/update_separate_loop_metadata.ll:1
+; Tests loop-simplify updates the newly created outter loop metadata with
+; a new loop id and keeps other MDNode as before.
----------------
typo: outer


================
Comment at: llvm/test/Transforms/LoopSimplify/update_separate_loop_metadata.ll:1
+; Tests loop-simplify updates the newly created outter loop metadata with
+; a new loop id and keeps other MDNode as before.
----------------
bjope wrote:
> typo: outer
"Test that loop-simplify ..." ?


================
Comment at: llvm/test/Transforms/LoopSimplify/update_separate_loop_metadata.ll:4
+
+; RUN: opt < %s -passes=loop-simplify -S | FileCheck %s
+; CHECK-LABEL: while.body.thread:
----------------
I guess this kind of verification could be sufficient.
Another idea is to use `-passes='loop-simplify,print<loops> -o /dev/null 2>&1'` and then check that the loop info printed identify two nested loops.

But maybe we want to check that the metadata "survives" as well (if the goal is that both loops should have the same metadata but identify as different loop). Then we need to CHECK the actual metadata as well (to see that both loops are annotated with "llvm.loop.usefulinfo").


================
Comment at: llvm/test/Transforms/LoopSimplify/update_separate_loop_metadata.ll:48
+!6 = !{!"llvm.loop.usefulinfo"}                   ; MDString used to represent any metadata
\ No newline at end of file

----------------
No newline at end of file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136102