<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 12, 2014 at 11:44 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Jun 12, 2014 at 11:15 AM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Jun 12, 2014 at 2:43 AM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
>><br>
>> ----- Original Message -----<br>
>> > From: "Eli Bendersky" <<a href="mailto:eliben@google.com">eliben@google.com</a>><br>
>> > To: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > Sent: Wednesday, June 11, 2014 6:15:35 PM<br>
>> > Subject: [llvm] r210721 - Teach LoopUnrollPass to respect loop unrolling<br>
>> > hints in metadata.<br>
>> ><br>
>> > Author: eliben<br>
>> > Date: Wed Jun 11 18:15:35 2014<br>
>> > New Revision: 210721<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=210721&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=210721&view=rev</a><br>
>> > Log:<br>
>> > Teach LoopUnrollPass to respect loop unrolling hints in metadata.<br>
>> ><br>
>> > See <a href="http://reviews.llvm.org/D4090" target="_blank">http://reviews.llvm.org/D4090</a> for more details.<br>
>> ><br>
>> > The Clang change that produces this metadata was committed in r210667<br>
>> ><br>
>> > Patch by Mark Heffernan.<br>
>> ><br>
>> ><br>
>> > Added:<br>
>> > llvm/trunk/test/Transforms/LoopUnroll/unroll-pragmas.ll<br>
>> > Modified:<br>
>> > llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp<br>
>> ><br>
>> > Modified: llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp<br>
>> > URL:<br>
>> ><br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp?rev=210721&r1=210720&r2=210721&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp?rev=210721&r1=210720&r2=210721&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp (original)<br>
>> > +++ llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp Wed Jun 11<br>
>> > 18:15:35 2014<br>
>> > @@ -20,6 +20,7 @@<br>
>> > #include "llvm/IR/DataLayout.h"<br>
>> > #include "llvm/IR/Dominators.h"<br>
>> > #include "llvm/IR/IntrinsicInst.h"<br>
>> > +#include "llvm/IR/Metadata.h"<br>
>> > #include "llvm/Support/CommandLine.h"<br>
>> > #include "llvm/Support/Debug.h"<br>
>> > #include "llvm/Support/raw_ostream.h"<br>
>> > @@ -36,7 +37,8 @@ UnrollThreshold("unroll-threshold", cl::<br>
>> ><br>
>> > static cl::opt<unsigned><br>
>> > UnrollCount("unroll-count", cl::init(0), cl::Hidden,<br>
>> > - cl::desc("Use this unroll count for all loops, for testing<br>
>> > purposes"));<br>
>> > + cl::desc("Use this unroll count for all loops including those with<br>
>> > "<br>
>> > + "unroll_count pragma values, for testing purposes"));<br>
>> ><br>
>> > static cl::opt<bool><br>
>> > UnrollAllowPartial("unroll-allow-partial", cl::init(false),<br>
>> > cl::Hidden,<br>
>> > @@ -47,6 +49,13 @@ static cl::opt<bool><br>
>> > UnrollRuntime("unroll-runtime", cl::ZeroOrMore, cl::init(false),<br>
>> > cl::Hidden,<br>
>> > cl::desc("Unroll loops with run-time trip counts"));<br>
>> ><br>
>> > +// Maximum allowed unroll count for a loop being fully unrolled<br>
>> > +// because of a pragma unroll(enable) statement (ie, metadata<br>
>> > +// "llvm.loopunroll.enable" is true). This prevents unexpected<br>
>> > +// behavior like crashing when using this pragma on high trip count<br>
>> > +// loops.<br>
>> > +static const unsigned PragmaFullUnrollCountLimit = 1024;<br>
>><br>
>> Please make this a cl::opt! Also, it needs to be a limit on the final loop<br>
>> size, not on the unroll count itself (see the PartialThreshold variable near<br>
>> the top of runOnLoop).<br>
>><br>
>> > +<br>
>> > namespace {<br>
>> > class LoopUnroll : public LoopPass {<br>
>> > public:<br>
>> > @@ -151,6 +160,63 @@ static unsigned ApproximateLoopSize(cons<br>
>> > return LoopSize;<br>
>> > }<br>
>> ><br>
>> > +// Returns the value associated with the given metadata node name<br>
>> > (for<br>
>> > +// example, "llvm.loopunroll.count"). If no such named metadata<br>
>> > node<br>
>> > +// exists, then nullptr is returned.<br>
>> > +static const ConstantInt *GetUnrollMetadataValue(const Loop *L,<br>
>> > + StringRef Name) {<br>
>> > + MDNode *LoopID = L->getLoopID();<br>
>> > + if (!LoopID) return nullptr;<br>
>> > +<br>
>> > + // First operand should refer to the loop id itself.<br>
>> > + assert(LoopID->getNumOperands() > 0 && "requires at least one<br>
>> > operand");<br>
>> > + assert(LoopID->getOperand(0) == LoopID && "invalid loop id");<br>
>> > +<br>
>> > + for (unsigned i = 1, e = LoopID->getNumOperands(); i < e; ++i) {<br>
>> > + const MDNode *MD = dyn_cast<MDNode>(LoopID->getOperand(i));<br>
>> > + if (!MD) continue;<br>
>> > +<br>
>> > + const MDString *S = dyn_cast<MDString>(MD->getOperand(0));<br>
>> > + if (!S) continue;<br>
>> > +<br>
>> > + if (Name.equals(S->getString())) {<br>
>> > + assert(MD->getNumOperands() == 2 &&<br>
>> > + "Unroll hint metadata should have two operands.");<br>
>> > + return cast<ConstantInt>(MD->getOperand(1));<br>
>> > + }<br>
>> > + }<br>
>> > + return nullptr;<br>
>> > +}<br>
>> > +<br>
>> > +// Returns true if the loop has an unroll(enable) pragma.<br>
>> > +static bool HasUnrollEnablePragma(const Loop *L) {<br>
>> > + const ConstantInt *EnableValue =<br>
>> > + GetUnrollMetadataValue(L, "llvm.loopunroll.enable");<br>
>> > + return (EnableValue && EnableValue->getZExtValue());<br>
>> > + return false;<br>
>> > +}<br>
>> > +<br>
>> > +// Returns true if the loop has an unroll(disable) pragma.<br>
>> > +static bool HasUnrollDisablePragma(const Loop *L) {<br>
>> > + const ConstantInt *EnableValue =<br>
>> > + GetUnrollMetadataValue(L, "llvm.loopunroll.enable");<br>
>> > + return (EnableValue && !EnableValue->getZExtValue());<br>
>> > + return false;<br>
>> > +}<br>
>> > +<br>
>> > +// Check for unroll_count(N) pragma. If found, return true and set<br>
>> > +// Count to the integer parameter of the pragma.<br>
>> > +static bool HasUnrollCountPragma(const Loop *L, int &Count) {<br>
>> > + const ConstantInt *CountValue =<br>
>> > + GetUnrollMetadataValue(L, "llvm.loopunroll.count");<br>
>> > + if (CountValue) {<br>
>> > + Count = CountValue->getZExtValue();<br>
>> > + assert(Count >= 1 && "Unroll count must be positive.");<br>
>> > + return true;<br>
>> > + }<br>
>> > + return false;<br>
>> > +}<br>
>> > +<br>
>> > bool LoopUnroll::runOnLoop(Loop *L, LPPassManager &LPM) {<br>
>> > if (skipOptnoneFunction(L))<br>
>> > return false;<br>
>> > @@ -202,12 +268,49 @@ bool LoopUnroll::runOnLoop(Loop *L, LPPa<br>
>> > TripMultiple = SE->getSmallConstantTripMultiple(L, LatchBlock);<br>
>> > }<br>
>> ><br>
>> > - bool Runtime = UserRuntime ? CurrentRuntime : UP.Runtime;<br>
>> > + // User-specified count (either as a command-line option or<br>
>> > + // constructor parameter) has highest precedence.<br>
>> > + unsigned Count = UserCount ? CurrentCount : 0;<br>
>> > +<br>
>> > + // If there is no user-specified count, unroll pragmas have the<br>
>> > next<br>
>> > + // highest precendence.<br>
>> > + if (Count == 0) {<br>
>> > + if (HasUnrollDisablePragma(L)) {<br>
>> > + // Loop has unroll(disable) pragma.<br>
>> > + return false;<br>
>> > + }<br>
>> ><br>
>> > - // Use a default unroll-count if the user doesn't specify a value<br>
>> > - // and the trip count is a run-time value. The default is<br>
>> > different<br>
>> > - // for run-time or compile-time trip count loops.<br>
>> > - unsigned Count = UserCount ? CurrentCount : UP.Count;<br>
>> > + int PragmaCount;<br>
>> > + if (HasUnrollCountPragma(L, PragmaCount)) {<br>
>> > + if (PragmaCount == 1) {<br>
>> > + // Nothing to do.<br>
>> > + return false;<br>
>> > + }<br>
>> > + Count = PragmaCount;<br>
>> > + Threshold = NoThreshold;<br>
>> > + } else if (HasUnrollEnablePragma(L)) {<br>
>> > + // Loop has unroll(enable) pragma without a unroll_count<br>
>> > pragma,<br>
>> > + // so unroll loop fully if possible.<br>
>> > + if (TripCount == 0) {<br>
>> > + DEBUG(dbgs() << " Loop has unroll(enable) pragma but loop<br>
>> > cannot be "<br>
>> > + "fully unrolled because trip count is<br>
>> > unknown.\n");<br>
>><br>
>> This should not say "fully".<br>
>><br>
>> > + // Continue with standard heuristic unrolling.<br>
>> > + } else if (TripCount > PragmaFullUnrollCountLimit) {<br>
>> > + DEBUG(dbgs() << " Loop has unroll(enable) pragma but loop<br>
>> > cannot be "<br>
>> > + "fully unrolled because loop count is<br>
>> > greater than "<br>
>> > + << PragmaFullUnrollCountLimit);<br>
>> > + // Continue with standard heuristic unrolling.<br>
>><br>
>><br>
>> This should also not say "fully". In addition, this is misleading because<br>
>> we might unroll this anyway (just not to the extent requested by the user.<br>
>><br>
>> Also, I think that we should be using emitOptimizationRemark here (like<br>
>> LoopVectorize does) instead of debug messages.<br>
>><br>
>> Thanks again,<br>
>> Hal<br>
>><br>
><br>
> Thanks Hal. We'll address the comments once the patch is revised and gets<br>
> back in.<br>
><br>
<br>
</div></div>Well, since it's out you might as well address them before it goes back in.</blockquote><div><br></div><div>That's what I meant, of course :) Sorry if I wasn't clear.</div><div><br></div><div>Eli </div>
</div><br></div></div>