This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Honor '#pragma unroll' even with -fno-unroll-loops.
ClosedPublic

Authored by Meinersbur on Dec 14 2018, 1:29 PM.

Details

Summary

When using clang with -fno-unroll-loops (implicitly added with -O1), the LoopUnrollPass is not not added to the (legacy) pass pipeline. This also means that it will not process any loop metadata such as llvm.loop.unroll.enable (which is generated by #pragma unroll or #pragma clang loop unroll(enable)) and remain in the IR until the WarnMissedTransformationsPass emits a warning that a forced transformation has not been applied (see https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20181210/610833.html). Such explicit transformations should take precedence over disabling heuristics.

This patch unconditionally adds LoopUnrollPass to the optimizing pipeline (i.e. it is not added with -O0), but passes a flag indicating whether automatic unrolling is dis-/enabled. This is the same approach as LoopVectorize uses.

The new pass manager's pipeline builder has no option to disable unrolling, hence the problem does not apply.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur created this revision.Dec 14 2018, 1:29 PM
Meinersbur marked an inline comment as done.Dec 14 2018, 1:33 PM
Meinersbur added inline comments.
lib/Transforms/IPO/PassManagerBuilder.cpp
695 ↗(On Diff #178269)

Should these passes be added unconditionally as well?

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.

include/llvm/Transforms/Scalar/LoopUnrollPass.h
26 ↗(On Diff #178269)

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?

lib/Transforms/IPO/PassManagerBuilder.cpp
695 ↗(On Diff #178269)

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.

  • Rename AlwaysUnroll -> !OnlyWhenForced
  • Add comments describing OnlyWhenForced
This revision is now accepted and ready to land.Dec 17 2018, 12:17 PM
This revision was automatically updated to reflect the committed changes.