[PATCH] D50698: [UnJ] Ensure unroll_and_jam metadata is removed once consumed.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/llvm/Analysis/LoopInfo.h:568
   /// unrolling pass is run more than once (which it generally is).
-  void setLoopAlreadyUnrolled();
+  void setLoopAlreadyUnrolled(StringRef Prefix = "llvm.loop.unroll.");
 
----------------
hiraditya wrote:
> I think we can add the Prefix at the caller all the time, a default value of "llvm.loop.unroll." may not be general. It seems renaming the function to reflect a more general semantics like: setLoopAlreadyTransformed and then renaming appropriate local variables would be a good improvement.
There are a lot of inconsistencies in handling metadata. LoopVectorizer adds `llvm.loop.isvectorized` instead of removing its metadata. LoopDistribution doesn't care about remaining metadata at all. 

I am trying to add a bit of uniformness in D49281. If a followup attribute is set, it drops all loop preexisting metadata since the transformed loop is a different loop than the loop the metadata is for.

LoopUnroll and LoopUnrollAndJam are strangely intertwined. Some setting to LoopUnroll also apply on LoopUnrollAndJam. Not an ideal situation.

If we go for a different name, I'd suggest `forceDisableTransformation` or similar. `setLoopAlreadyUnrolled` sounds like it does just sets a flag, but does more.


================
Comment at: lib/Analysis/LoopInfo.cpp:295
   SmallVector<Metadata *, 1> DisableOperands;
   DisableOperands.push_back(MDString::get(Context, "llvm.loop.unroll.disable"));
   MDNode *DisableNode = MDNode::get(Context, DisableOperands);
----------------
[serious] This is intended to disable unroll-and-jam as well? why not using `Prefix + "disable"`?


https://reviews.llvm.org/D50698