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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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.");
 
----------------
Meinersbur wrote:
> 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.
As this is only used for unroll and unroll and jam, I think setLoopAlreadyUnrolled still fits, at least for the moment.

I think that once it's in it would be good to move towards the function in D49281, which should be able to handle this in a more generic way. This patch is an attempt to make that work better and not produce warnings for unrolled and jammed loops.


================
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);
----------------
Meinersbur wrote:
> [serious] This is intended to disable unroll-and-jam as well? why not using `Prefix + "disable"`?
Yep, it was intended. But with Prefix may be better.

llvm.loop.unroll.disable also disables unroll-and-jam if there is no unroll_and_jam metadata. As we remove all the unroll_and_jam metadata above, this will disable any further unrolling or unroll-and-jamming.

Whether it is entirely necessary to disable unroll here, I'm not 100% sure of. If a user does:
  #pragma unroll_and_jam(2)
  for ..
    #pragma unroll
    for ..
I'd expect it to be unroll-and-jammed by 2, but not then unrolled more. Using "llvm.loop.unroll.disable" will disable any follow-on unroll or unroll-and-jam.


https://reviews.llvm.org/D50698