Page MenuHomePhabricator

Fix bug causing loop unrolling to be disabled in some cases for loops with '#pragma clang loop unroll_count(N)"
ClosedPublic

Authored by meheff on Jul 24 2014, 2:59 PM.

Details

Reviewers
hfinkel
Summary

After unrolling a loop with llvm.loop.unroll.count metadata (unroll factor hint) the loop unroller replaces the llvm.loop.unroll.count metadata with llvm.loop.unroll.disable metadata to prevent any subsequent unrolling passes from unrolling more than the hint indicates. However, if the unroll count metadata is shared between loops then unrolling for these other loops will also be disabled. This small patch fixes that problem by adding the llvm.loop.unroll.disable metadata to only the loop which was just unrolled.

Diff Detail

Event Timeline

meheff updated this revision to Diff 11856.Jul 24 2014, 2:59 PM
meheff retitled this revision from to Fix bug causing loop unrolling to be disabled in some cases for loops with '#pragma clang loop unroll_count(N)".
meheff updated this object.
meheff edited the test plan for this revision. (Show Details)
meheff added a reviewer: hfinkel.
meheff added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Jul 24 2014, 3:03 PM

In true LLVM style, I ask for a 40-line test case for your one-line change ;)

-Hal

  • Original Message -----

From: "Mark Heffernan" <meheff@google.com>
To: meheff@google.com, hfinkel@anl.gov
Cc: llvm-commits@cs.uiuc.edu
Sent: Thursday, July 24, 2014 4:59:24 PM
Subject: [PATCH] Fix bug causing loop unrolling to be disabled in some cases for loops with '#pragma clang loop
unroll_count(N)"

Hi hfinkel,

After unrolling a loop with llvm.loop.unroll.count metadata (unroll
factor hint) the loop unroller replaces the llvm.loop.unroll.count
metadata with llvm.loop.unroll.disable metadata to prevent any
subsequent unrolling passes from unrolling more than the hint
indicates. However, if the unroll count metadata is shared between
loops then unrolling for these other loops will also be disabled.

This small patch fixes that problem by adding the

llvm.loop.unroll.disable metadata to only the loop which was just
unrolled.

http://reviews.llvm.org/D4662

Files:

lib/Transforms/Scalar/LoopUnrollPass.cpp

Index: lib/Transforms/Scalar/LoopUnrollPass.cpp
===================================================================

    • lib/Transforms/Scalar/LoopUnrollPass.cpp +++ lib/Transforms/Scalar/LoopUnrollPass.cpp @@ -302,7 +302,6 @@ // Set operand 0 to refer to the loop id itself. NewLoopID->replaceOperandWith(0, NewLoopID); L->setLoopID(NewLoopID);
  • LoopID->replaceAllUsesWith(NewLoopID); }

    unsigned LoopUnroll::selectUnrollCount(
  • Original Message -----

From: "Hal Finkel" <hfinkel@anl.gov>
To: reviews+D4662+public+09cbf8b56d9abfd0@reviews.llvm.org
Cc: llvm-commits@cs.uiuc.edu
Sent: Thursday, July 24, 2014 5:02:48 PM
Subject: Re: [PATCH] Fix bug causing loop unrolling to be disabled in some cases for loops with '#pragma clang loop
unroll_count(N)"

In true LLVM style, I ask for a 40-line test case for your one-line
change ;)

To be clear, otherwise, LGTM!

Thanks,
Hal

-Hal
  • Original Message ----- > From: "Mark Heffernan" <meheff@google.com> > To: meheff@google.com, hfinkel@anl.gov > Cc: llvm-commits@cs.uiuc.edu > Sent: Thursday, July 24, 2014 4:59:24 PM > Subject: [PATCH] Fix bug causing loop unrolling to be disabled in > some cases for loops with '#pragma clang loop > unroll_count(N)" > > Hi hfinkel, > > After unrolling a loop with llvm.loop.unroll.count metadata (unroll > factor hint) the loop unroller replaces the llvm.loop.unroll.count > metadata with llvm.loop.unroll.disable metadata to prevent any > subsequent unrolling passes from unrolling more than the hint > indicates. However, if the unroll count metadata is shared between > loops then unrolling for these other loops will also be disabled. > This small patch fixes that problem by adding the > llvm.loop.unroll.disable metadata to only the loop which was just > unrolled. > > http://reviews.llvm.org/D4662 > > Files: > lib/Transforms/Scalar/LoopUnrollPass.cpp > > Index: lib/Transforms/Scalar/LoopUnrollPass.cpp > =================================================================== > --- lib/Transforms/Scalar/LoopUnrollPass.cpp > +++ lib/Transforms/Scalar/LoopUnrollPass.cpp > @@ -302,7 +302,6 @@ > // Set operand 0 to refer to the loop id itself. > NewLoopID->replaceOperandWith(0, NewLoopID); > L->setLoopID(NewLoopID); > - LoopID->replaceAllUsesWith(NewLoopID); > } > > unsigned LoopUnroll::selectUnrollCount( >

    -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
meheff updated this revision to Diff 11859.Jul 24 2014, 3:29 PM
meheff edited edge metadata.

Thanks! Test added. Will lead with that next time ;-)

hfinkel accepted this revision.Jul 24 2014, 3:31 PM
hfinkel edited edge metadata.

Thanks! (LGTM, again!)

This revision is now accepted and ready to land.Jul 24 2014, 3:31 PM

Committed as r213900.

Mark

meheff closed this revision.Apr 16 2015, 10:25 AM