[PATCH] D55716: [LoopUnroll] Honor '#pragma unroll' even with -fno-unroll-loops.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

> Such explicit transformations should take precedence over disabling heuristics.

I agree. This was our consensus for the vectorization pragmas (that they should override the general enablement provided by command-line flags), and I see no reason why loop unrolling should be any different in this regard.



================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:26
   const int OptLevel;
+  const bool AlwaysUnroll;
 
----------------
I don't like this name "AlwaysUnroll" as it sounds like a forced action for all loops, and it's really a pass-enablement flag. I understand, however, that the corresponding flag in the loop vectorizer is named this:

  /// If true, consider all loops for vectorization.
  /// If false, only loops that explicitly request vectorization are
  /// considered.
  bool AlwaysVectorize = true;

which is also a suboptimal name, but as a comment which explains what it really means. You should have a similar comment here. Maybe a better name would be: OnlyWhenForced?



================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:695
 
+  if (!DisableUnrollLoops) {
     // LoopUnroll may generate some redundency to cleanup.
----------------
Meinersbur wrote:
> Should these passes be added unconditionally as well?
In theory, you'd want these in the pipline for loops where unrolling is enabled. In practice, that's going to cause a larger variation from this change than the actually loop-unrolling changes, and so, if we want to do that, it should be a separate patch. For now, I'd leave these unchanged.


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

https://reviews.llvm.org/D55716