[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: 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);
----------------
dmgreen wrote:
> 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.
I'd prefer to minimize the interaction between different passes. I am understanding unroll-and-jam as a pass orthogonal to unroll. I have no model in what why their metadata interact, and I'd prefer if it was predictable.

At the very least, please add a comment about the intention; it look too much like something has been forgotten.

> 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.

I don't understand. This this patch is already using `llvm.loop.unroll.disable` and disables further unrolling of the //outer// loop. There is no effect on the inner loop. That is, your example works as expected (I tried).



https://reviews.llvm.org/D50698