[PATCH] D49281: [Unroll/UnrollAndJam/Vectorizer/Distribute] Add followup loop attributes.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

I agree that this is a very powerful idea (something I wish I'd had back when I was writing psuedo-hpc applications). I think it's well worth having, but equally worth making sure we get it right. The clang side is very important for that.

In https://reviews.llvm.org/D49281#1188441, @Meinersbur wrote:

> In https://reviews.llvm.org/D49281#1187583, @dmgreen wrote:
>
> > Some extra tests for nonforced + a pragma would be good to see.
>
>
> Any transformations in particular?


There seem to be tests that nonforced disables things, but not that nonforced + an attribute keeps it enabled. e.g. the unroll.count metadata.



================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:753
+  if (!PragmaEnableUnroll && hasDisableAllTransformsHint(L))
+    return false;
   bool ExplicitUnroll = PragmaCount > 0 || PragmaFullUnroll ||
----------------
Meinersbur wrote:
> dmgreen wrote:
> > What if it has PragmaCount and hasDisableAllTransformsHint? Should that not enable too?
> If `llvm.loop.unroll.enable` is not set, interpretation here is that the transformation is 'non-forced', that is, `llvm.loop.unroll.count` is a hint to the compiler that if it unrolls, then it should unroll by that amount. `llvm.loop.disable_nonforced` overrride the decision whether to unroll, i.e. the unroll factor does not matter.
> 
> I am aware that the concept of 'forced' transformations is not consistent between passes, but I am trying to give it some consistency. Passes could query shared code such as `hasUnrollTransformation` in `LoopUtils`. `hasUnrollTransformation` currently follows your interpretation of `llvm.loop.unroll.count` / `llvm.loop.disable_nonforced`. I am happy to implement either definition, as long as we find a consistent rule.
I would expect that if a loop has any metadata for a pass, that would mean disable_nonforced doesn't apply. As if the user has specified some metadata, it likely wants something to happen.

I think in this specific case llvm.loop.unroll.count implies llvm.loop.unroll.enable, and we wouldn't put both on a loop for "#pragma unroll(4)" or "#pragma clang loop unroll_count(4)"


================
Comment at: test/Transforms/LoopUnroll/unroll-pragmas_transform.ll:1
+; RUN: opt < %s -loop-unroll -pragma-unroll-threshold=1024 -S | FileCheck -check-prefixes=CHECK,REM %s
+; RUN: opt < %s -loop-unroll -loop-unroll -pragma-unroll-threshold=1024 -S | FileCheck -check-prefixes=CHECK,REM %s
----------------
Meinersbur wrote:
> dmgreen wrote:
> > This file look sensible on it's own and I think looks OK to be committed separately. (Apart from the nit below)
> This is a copy of `unroll-pragmas.ll` and any ambiguous metadata replaced by follow-up attributes.
> 
> An hope is to generally make 'multiple transformation attributes on the same loop' illegal and rejected by the IR verifier (since the result depends on an implementation detail: the order in the pass manager). In this case this file would replace `unroll-pragmas.ll`.
> 
> But my expectation is that we cannot break backwards-compatibility this way.
Ah, I missed the "followup" here. Is it worth replicating this entire file, or should it just be an extra test in the old file.  
The "followup" on unroll_1 seems to be the only test changed here? To add unroll.disable as a followup attribute? I'm not sure I see why. Would we expect "#pragma unroll(1)" to not work as it did before? (disable unroll)


Repository:
  rL LLVM

https://reviews.llvm.org/D49281