Page MenuHomePhabricator

[LoopUnroll] Push runtime unrolling decision up into tryToUnrollLoop()
ClosedPublic

Authored by nikic on Jun 17 2021, 1:44 PM.

Details

Summary

Currently, UnrollLoop() is passed an AllowRuntime flag and decides itself whether runtime unrolling should be used or not. This patch pushes the decision into the caller and allows us to eliminate the ULO.TripCount and ULO.TripMultiple parameters.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > LLVM.CodeGen/AArch64::loop-micro-op-buffer-size-t99.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -mcpu=thunderx2t99 -passes=loop-unroll --debug-only=loop-unroll --debug-only=basicblock-utils -S -unroll-allow-partial < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/loop-micro-op-buffer-size-t99.ll 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/loop-micro-op-buffer-size-t99.ll

Event Timeline

nikic created this revision.Jun 17 2021, 1:44 PM
nikic requested review of this revision.Jun 17 2021, 1:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 1:44 PM
nikic updated this revision to Diff 352836.Jun 17 2021, 1:46 PM

Fix comment typos.

nikic added inline comments.Jun 17 2021, 1:50 PM
llvm/lib/Transforms/Utils/LoopUnroll.cpp
391

I'm not particularly familiar with convergent operations, but looking back at D17526 the concern here seems to have been specifically about the prologue introduced by runtime unrolling, not anything else.

llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
251

This change deserves a comment: If TripMultiple is 1 and Count is 1 and UnrollRemainder was set, this tried to unroll the remainder loop with a Count of 0, which is not legal. It wasn't caught previously due to the check

// Don't enter the unroll code if there is nothing to do.
if (ULO.TripCount == 0 && ULO.Count < 2) {

which happened before the assert(Count > 0) assertion. I've removed that check and the assertion is triggered now.

The change here adjust things to not try to create a remainder loop for an unroll count of 1, as this doesn't make sense in the first place.

reames accepted this revision.Jun 18 2021, 9:43 AM

LGTM. Nice cleanup.

This revision is now accepted and ready to land.Jun 18 2021, 9:43 AM
This revision was landed with ongoing or failed builds.Jun 19 2021, 12:32 AM
This revision was automatically updated to reflect the committed changes.