This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Prevent LoopFullUnrollPass to perform partial/runtime unrolling
ClosedPublic

Authored by yassingh on Apr 12 2023, 12:27 AM.

Details

Summary

FullLoopUnroll was performing runtime unrolling in certain cases when
'#pragma unroll' was specified. Patch to fix this by introducing new parameter
to tryToUnrollLoop() to differentiate between LoopUnrollPass and
FullLoopUnrollPass. Based on the discussion here
(https://discourse.llvm.org/t/loop-unroller-fails-to-unroll-loop/69834)

Diff Detail

Event Timeline

yassingh created this revision.Apr 12 2023, 12:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 12:27 AM
yassingh requested review of this revision.Apr 12 2023, 12:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 12:27 AM
yassingh edited the summary of this revision. (Show Details)Apr 12 2023, 12:33 AM
yassingh added reviewers: nikic, reames, fhahn, efriedma.
LLVM :: Transforms/LoopUnroll/full-unroll-keep-first-exit.ll
LLVM :: Transforms/LoopUnroll/revisit.ll

These are the 2 tests that I think should be updated.

nikic added inline comments.Apr 12 2023, 12:58 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
1127

isFullUnroll -> OnlyFullUnroll?

1309

This probably isn't quite the right condition. I can think of two problematic cases here:

  • We know the trip count, but it's very large, so we will only perform a partial unroll. We don't want to do that during FullUnroll.
  • We know the max trip count and unroll to the max trip count, removing the loop. I think we consider this a full unroll as well.

I believe our criterion for what counts as a full unroll is basically "is the unroll count >= max trip count".

1311

parital -> partial

yassingh updated this revision to Diff 512712.Apr 12 2023, 2:24 AM
yassingh edited the summary of this revision. (Show Details)
  • Addressed review comments
nikic accepted this revision.Apr 12 2023, 2:39 AM

LGTM, assuming this passes tests.

This revision is now accepted and ready to land.Apr 12 2023, 2:39 AM
arsenm added inline comments.Apr 12 2023, 3:44 AM
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
1312

No reason to break the newline out of the string unless you are going to print additional information before it

yassingh updated this revision to Diff 512783.Apr 12 2023, 5:02 AM
  • updated failing tests
  • fixed debug string
yassingh added inline comments.Apr 12 2023, 5:05 AM
llvm/test/Transforms/LoopUnroll/revisit.ll
11

How can I add an appropriate test for this option "unroll-revisit-child-loops"?

80–156

Removed this test, as full-unroll no longer performs partial unrolling when forced.

nikic added inline comments.Apr 12 2023, 6:12 AM
llvm/test/Transforms/LoopUnroll/full-unroll-keep-first-exit.ll
3

Can you please restore this check line, possibly with an additional prefix?

llvm/test/Transforms/LoopUnroll/revisit.ll
11

I think you can just drop the option entirely, it no longer makes sense. We can probably assert(!IsCurrentLoopValid) now: Full unrolling should always invalidate the current loop.

yassingh updated this revision to Diff 512813.Apr 12 2023, 6:38 AM
  • Review comments
yassingh marked an inline comment as done.Apr 12 2023, 6:39 AM
yassingh added inline comments.
llvm/test/Transforms/LoopUnroll/revisit.ll
11

Removed this run line. I can submit another change to entirely drop this cl option.

nikic added inline comments.Apr 12 2023, 6:41 AM
llvm/test/Transforms/LoopUnroll/full-unroll-keep-first-exit.ll
3

This should use --check-prefixes=CHECK,CHECK-FULL to deduplicate the cases that are the same between full/non-full.

llvm/test/Transforms/LoopUnroll/revisit.ll
11

That's fine.

yassingh updated this revision to Diff 512821.Apr 12 2023, 6:58 AM
  • Updated test, didn't need to update the check prefix. Passing now.
nikic accepted this revision.Apr 12 2023, 7:01 AM

LGTM

llvm/test/Transforms/LoopUnroll/full-unroll-keep-first-exit.ll
436

Spurious change. Sounds like you can just leave this test alone.