<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div style>Hey,</div><div style><br></div><div style>This patch causes LLVM to segfault on certain loops that look almost like popcount... I don't have a reduced test case but it should be very easy to write one. However, there are pretty pervasive problems with this patch. I think combined with the two bugs I spotted by inspection, the failure Galina posted, and the numerous style problems, you should probably revert until it's in better shape.</div>
<div style><br></div><div style>I've provided as detailed of comments as I could below to try to help you improve this. The bugs are toward the end, and I'm afraid I ran out of steam toward the end in terms of reviewing as well.</div>
<div><br></div>On Thu, Nov 29, 2012 at 11:38 AM, Shuxin Yang <span dir="ltr"><<a href="mailto:shuxin.llvm@gmail.com" target="_blank">shuxin.llvm@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Modified: llvm/trunk/include/llvm/Target/TargetTransformImpl.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Target/TargetTransformImpl.h?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Target/TargetTransformImpl.h?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Target/TargetTransformImpl.h (original)<br>
+++ llvm/trunk/include/llvm/Target/TargetTransformImpl.h Thu Nov 29 13:38:54 2012<br>
@@ -26,7 +26,7 @@<br>
/// ScalarTargetTransformInfo interface. Different targets can implement<br>
/// this interface differently.<br>
class ScalarTargetTransformImpl : public ScalarTargetTransformInfo {<br>
-private:<br>
+protected:<br>
const TargetLowering *TLI;<br>
<br>
public:<br>
<br>
Modified: llvm/trunk/include/llvm/TargetTransformInfo.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TargetTransformInfo.h?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TargetTransformInfo.h?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/TargetTransformInfo.h (original)<br>
+++ llvm/trunk/include/llvm/TargetTransformInfo.h Thu Nov 29 13:38:54 2012<br>
@@ -75,6 +75,18 @@<br>
/// LSR, and LowerInvoke use this interface.<br>
class ScalarTargetTransformInfo {<br>
public:<br>
+ /// PopcntHwSupport - Hardware support for population count. Compared to the<br>
+ /// SW implementation, HW support is supposed to significantly boost the<br>
+ /// performance when the population is dense, and it may or not may degrade<br></blockquote><div><br></div><div style>"it may or may not" rather than "it may or not may".</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /// performance if the population is sparse. A HW support is considered as<br>
+ /// "Fast" if it can outperform, or is on a par with, SW implementaion when<br>
+ /// the population is sparse; otherwise, it is considered as "Slow".<br></blockquote><div><br></div><div style>I wonder, why not express this in the interface? You could query for fast/slow with an expected density?</div>
<div style><br></div><div style>Also, some of this comment might be more naturally structured on the actual method rather than on the return type.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ enum PopcntHwSupport {<br>
+ None,<br>
+ Fast,<br>
+ Slow<br>
+ };<br></blockquote><div><br></div><div style>Please follow the coding standards for naming enum types and enumerators.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
virtual ~ScalarTargetTransformInfo() {}<br>
<br>
/// isLegalAddImmediate - Return true if the specified immediate is legal<br>
@@ -122,6 +134,11 @@<br>
virtual bool shouldBuildLookupTables() const {<br>
return true;<br>
}<br>
+<br>
+ /// getPopcntHwSupport - Return hardware support for population count.<br></blockquote><div><br></div><div style>Please follow the doxygen examples in the coding standards and don't duplicate function names in comments.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ virtual PopcntHwSupport getPopcntHwSupport(unsigned IntTyWidthInBit) const {<br></blockquote><div><br></div><div style>Why accept the integer type width? does it matter other than it being a legal integer width for the target?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return None;<br>
+ }<br>
};<br>
<br>
/// VectorTargetTransformInfo - This interface is used by the vectorizers<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)<br>
+++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Thu Nov 29 13:38:54 2012<br>
@@ -17670,6 +17670,17 @@<br>
return -1;<br>
}<br>
<br>
+ScalarTargetTransformInfo::PopcntHwSupport<br>
+X86ScalarTargetTransformImpl::getPopcntHwSupport(unsigned TyWidth) const {<br>
+ assert(isPowerOf2_32(TyWidth) && "Ty width must be power of 2");</blockquote><div><br></div><div style>This constraint seems strange. It certainly doesn't seem like it should be an assert.</div><div style>
<br></div><div style>If i have an i24, i cant count the population of it by counting the population of it zext'ed to 32 bits...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ const X86Subtarget &ST = TLI->getTargetMachine().getSubtarget<X86Subtarget>();<br>
+<br>
+ // TODO: Currently the __builtin_popcount() implementation using SSE3<br>
+ // instructions is inefficient. Once the problem is fixed, we should<br>
+ // call ST.hasSSE3() instead of ST.hasSSE4().<br></blockquote><div><br></div><div style>__builtin_popcount isn't the issue here, this is in LLVM. What is the actual issue? Why isn't "Slow' used here?</div>
<div style><br></div><div style>Also LLVM uses FIXME in comments more than TODO.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return ST.hasSSE41() ? Fast : None;<br>
+}<br>
+<br>
unsigned<br>
X86VectorTargetTransformInfo::getArithmeticInstrCost(unsigned Opcode,<br>
Type *Ty) const {<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86ISelLowering.h (original)<br>
+++ llvm/trunk/lib/Target/X86/X86ISelLowering.h Thu Nov 29 13:38:54 2012<br>
@@ -933,6 +933,14 @@<br>
const TargetLibraryInfo *libInfo);<br>
}<br>
<br>
+ class X86ScalarTargetTransformImpl : public ScalarTargetTransformImpl {<br>
+ public:<br>
+ explicit X86ScalarTargetTransformImpl(const TargetLowering *TL) :<br>
+ ScalarTargetTransformImpl(TL) {};<br>
+<br>
+ virtual PopcntHwSupport getPopcntHwSupport(unsigned TyWidth) const;<br>
+ };<br>
+<br>
class X86VectorTargetTransformInfo : public VectorTargetTransformImpl {<br>
public:<br>
explicit X86VectorTargetTransformInfo(const TargetLowering *TL) :<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86TargetMachine.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86TargetMachine.h?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86TargetMachine.h?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86TargetMachine.h (original)<br>
+++ llvm/trunk/lib/Target/X86/X86TargetMachine.h Thu Nov 29 13:38:54 2012<br>
@@ -118,7 +118,7 @@<br>
X86SelectionDAGInfo TSInfo;<br>
X86TargetLowering TLInfo;<br>
X86JITInfo JITInfo;<br>
- ScalarTargetTransformImpl STTI;<br>
+ X86ScalarTargetTransformImpl STTI;<br>
X86VectorTargetTransformInfo VTTI;<br>
public:<br>
X86_64TargetMachine(const Target &T, StringRef TT,<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/LoopIdiomRecognize.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopIdiomRecognize.cpp?rev=168931&r1=168930&r2=168931&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopIdiomRecognize.cpp?rev=168931&r1=168930&r2=168931&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/LoopIdiomRecognize.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/LoopIdiomRecognize.cpp Thu Nov 29 13:38:54 2012<br>
@@ -56,6 +56,7 @@<br>
#include "llvm/Support/raw_ostream.h"<br>
#include "llvm/DataLayout.h"<br>
#include "llvm/Target/TargetLibraryInfo.h"<br>
+#include "llvm/TargetTransformInfo.h"<br>
#include "llvm/Transforms/Utils/Local.h"<br>
using namespace llvm;<br>
<br>
@@ -63,16 +64,83 @@<br>
STATISTIC(NumMemCpy, "Number of memcpy's formed from loop load+stores");<br>
<br>
namespace {<br>
+<br>
+ class LoopIdiomRecognize;<br>
+<br>
+ /// This class defines some utility functions for loop idiom recognization.<br></blockquote><div><br></div><div style>Please don't use a class to organize a collection of static methods. That is the role of a namespace.</div>
<div style><br></div><div style>And this doesn't need a namespace at all -- this is in a single .cc file. Please just use static functions, that is is the overwhelmingly predominant pattern in LLVM.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ class LIRUtil {<br>
+ public:<br>
+ /// Return true iff the block contains nothing but an uncondition branch<br>
+ /// (aka goto instruction).<br>
+ static bool isAlmostEmpty(BasicBlock *);<br></blockquote><div><br></div><div style>Please use names for parameters in LLVM.</div><div style><br></div><div style>Also, I don't think this needs a helper method. First, you *always* have a terminator instruction, the verify assures this. Typically, you would then test that &BB->begin() == BB->getTerminatorInst().</div>
<div style><br></div><div style>Or is the point of this function to test for an empty BB *and* an unconditional branch? If so, why do you need to handle that -- SimplifyCFG should have removed such blocks already?</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ static BranchInst *getBranch(BasicBlock *BB) {<br>
+ return dyn_cast<BranchInst>(BB->getTerminator());<br>
+ }<br></blockquote><div><br></div><div style>Abstracting this into a method doesn't seem to help readability at all. I would rather just see the dyn_cast.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /// Return the condition of the branch terminating the given basic block.<br>
+ static Value *getBrCondtion(BasicBlock *);<br></blockquote><div><br></div><div style>What if it doesn't terminate in a branch? I'm not really sure what abstraction you're trying to provide here.</div><div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /// Derive the precondition block (i.e the block that guards the loop<br>
+ /// preheader) from the given preheader.<br>
+ static BasicBlock *getPrecondBb(BasicBlock *PreHead);<br></blockquote><div><br></div><div style>I don't understand this.. What is the invariant it exposes? What does it test first? Why is this not a method on LoopInfo if this is generally useful?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ };<br>
+<br>
+ /// This class is to recoginize idioms of population-count conducted in<br>
+ /// a noncountable loop. Currently it only recognizes this pattern:<br>
+ /// \code<br>
+ /// while(x) {cnt++; ...; x &= x - 1; ...}<br>
+ /// \endcode<br>
+ class NclPopcountRecognize {<br></blockquote><div><br></div><div style>If NCL is an acronym you want to use, capitalize it as an acronym. I don't think it is a terribly useful acronym. I would call this just PopcountLoopRecognizer (note a noun, as this is a class), and document what sets of loops it handles currently. If there is some reason we wouldn't re-use this infrastructure for any future extensions, you should explain that here.</div>
<div style><br></div><div style><br></div><div style>Also, have you thought much about the design of these classes? Currently, we have LoopIdiomRecognize which is a pass, and which has innate logic for forming memset intrinsics out of loops. But we now also have a non-pass helper class to recognize a specific kind of loop. How would you envision this design scaling up to the next loop idiom?</div>
<div style><br></div><div style>I think that either the logic for recognizing popcount should be kept largely in methods on LoopIdiomRecognize (with helper static functions), or both it and memset recognition should be split out into nice patterned classes so that it is clear how to extend things.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ LoopIdiomRecognize &LIR;<br>
+ Loop *CurLoop;<br>
+ BasicBlock *PreCondBB;<br></blockquote><div><br></div><div style>If this is the only common state managed across the calls to various parts of your algorithm, I am increasingly of the opinion that you don't need an entire helper class here.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ typedef IRBuilder<> IRBuilderTy;<br></blockquote><div><br></div><div style>Why typedef this? Most everywhere just uses IRBuilder<> directly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ public:<br>
+ explicit NclPopcountRecognize(LoopIdiomRecognize &TheLIR);<br></blockquote><div><br></div><div style>As mentioned in other code reviews, just name this 'LIR'.</div><div style><br></div><div style>Also, when defining a small helper class, you don't need to define all of the methods out of line. Especially trivial ones should be defined inline.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ bool recognize(); </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ private:<br>
+ /// Take a glimpse of the loop to see if we need to go ahead recoginizing<br>
+ /// the idiom.<br>
+ bool preliminaryScreen();<br>
+<br>
+ /// Check if the given conditional branch is based on the comparison<br>
+ /// beween a variable and zero, and if the variable is non-zero, the<br>
+ /// control yeilds to the loop entry. If the branch matches the behavior,<br>
+ /// the variable involved in the comparion is returned. This function will<br>
+ /// be called to see if the precondition and postcondition of the loop<br>
+ /// are in desirable form.<br>
+ Value *matchCondition (BranchInst *Br, BasicBlock *NonZeroTarget) const;<br>
+<br>
+ /// Return true iff the idiom is detected in the loop. and 1) \p CntInst<br>
+ /// is set to the instruction counting the pupulation bit. 2) \p CntPhi<br>
+ /// is set to the corresponding phi node. 3) \p Var is set to the value<br>
+ /// whose population bits are being counted.<br>
+ bool detectIdiom<br>
+ (Instruction *&CntInst, PHINode *&CntPhi, Value *&Var) const;<br></blockquote><div><br></div><div style>Please never break a line after a function name in its declaration. This is very counterintuitive style in general, and deviates from all of the LLVM coding conventions.</div>
<div style><br></div><div style>I cannot stress this enough: it would make it *much* easier to read and understand your patches if you would spend some time learning how LLVM code is written, and trying to write code that matches the existing conventions and style. If we can help by extending the coding standards, please let us know.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /// Insert ctpop intrinsic function and some obviously dead instructions.<br>
+ void transform (Instruction *CntInst, PHINode *CntPhi, Value *Var);<br></blockquote><div><br></div><div style>No space before the '(' in a function call. This is in the standards.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /// Create llvm.ctpop.* intrinsic function.<br>
+ CallInst *createPopcntIntrinsic(IRBuilderTy &IRB, Value *Val, DebugLoc DL);</blockquote><div><br></div><div style>You shouldn't need your own routine for this, IRBuilder should do it for you. If it's helpers aren't sufficient, you should propose patches to enhance them.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+ };<br>
+<br>
class LoopIdiomRecognize : public LoopPass {<br>
Loop *CurLoop;<br>
const DataLayout *TD;<br>
DominatorTree *DT;<br>
ScalarEvolution *SE;<br>
TargetLibraryInfo *TLI;<br>
+ const ScalarTargetTransformInfo *STTI;<br>
public:<br>
static char ID;<br>
explicit LoopIdiomRecognize() : LoopPass(ID) {<br>
initializeLoopIdiomRecognizePass(*PassRegistry::getPassRegistry());<br>
+ TD = 0; DT = 0; SE = 0; TLI = 0; STTI = 0;<br></blockquote><div><br></div><div style>Why was this needed? Please always use the constructor's initializer list if you're going to set default values. And please don't write multiple assignment statements on a single line.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
<br>
bool runOnLoop(Loop *L, LPPassManager &LPM);<br>
@@ -110,6 +178,36 @@<br>
AU.addRequired<DominatorTree>();<br>
AU.addRequired<TargetLibraryInfo>();<br>
}<br>
+<br>
+ const DataLayout *getDataLayout() {<br>
+ return TD ? TD : TD=getAnalysisIfAvailable<DataLayout>();<br></blockquote><div><br></div><div style>No, don't query whether the analysis is available every time you go to get it. Look at how other passes do this. In the runOnFunction, walk through the class members that need to be initialized for that function and initialize them once. Then use the members everywhere rather than these accessors.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ DominatorTree *getDominatorTree() {<br>
+ return DT ? DT : (DT=&getAnalysis<DominatorTree>());<br>
+ }<br>
+<br>
+ ScalarEvolution *getScalarEvolution() {<br>
+ return SE ? SE : (SE = &getAnalysis<ScalarEvolution>());<br>
+ }<br>
+<br>
+ TargetLibraryInfo *getTargetLibraryInfo() {<br>
+ return TLI ? TLI : (TLI = &getAnalysis<TargetLibraryInfo>());<br>
+ }<br>
+<br>
+ const ScalarTargetTransformInfo *getScalarTargetTransformInfo() {<br>
+ if (!STTI) {<br>
+ TargetTransformInfo *TTI = getAnalysisIfAvailable<TargetTransformInfo>();<br>
+ if (TTI) STTI = TTI->getScalarTargetTransformInfo();<br>
+ }<br>
+ return STTI;<br>
+ }<br>
+<br>
+ Loop *getLoop() const { return CurLoop; }<br></blockquote><div><br></div><div style>If all of these accessors are only so that your helper class can reach in and use the members of the LoopIdiomRecognize pass, that is a sign that this is a bad design.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ private:<br>
+ bool runOnNoncountableLoop();<br>
+ bool runOnCountableLoop();<br>
};<br>
}<br>
<br>
@@ -172,24 +270,390 @@<br>
deleteDeadInstruction(I, SE, TLI);<br>
}<br>
<br>
-bool LoopIdiomRecognize::runOnLoop(Loop *L, LPPassManager &LPM) {<br>
- CurLoop = L;<br>
+//===----------------------------------------------------------------------===//<br>
+//<br>
+// Implementation of LIRUtil<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+// This fucntion will return true iff the given block contains nothing but goto.<br>
+// A typical usage of this function is to check if the preheader fucntion is<br>
+// "almost" empty such that generated intrinsic function can be moved across<br>
+// preheader and to be placed at the end of the preconditiona block without<br>
+// concerning of breaking data dependence.<br>
+bool LIRUtil::isAlmostEmpty(BasicBlock *BB) {<br>
+ if (BranchInst *Br = getBranch(BB)) {<br>
+ return Br->isUnconditional() && BB->size() == 1;<br>
+ }<br>
+ return false;<br>
+}<br>
<br>
- // If the loop could not be converted to canonical form, it must have an<br>
- // indirectbr in it, just give up.<br>
- if (!L->getLoopPreheader())<br>
+Value *LIRUtil::getBrCondtion(BasicBlock *BB) {<br>
+ BranchInst *Br = getBranch(BB);<br>
+ return Br ? Br->getCondition() : 0;<br>
+}<br>
+<br>
+BasicBlock *LIRUtil::getPrecondBb(BasicBlock *PreHead) {<br>
+ if (BasicBlock *BB = PreHead->getSinglePredecessor()) {<br>
+ BranchInst *Br = getBranch(BB);<br>
+ return Br && Br->isConditional() ? BB : 0;<br>
+ }<br>
+ return 0;<br>
+}<br>
+<br>
+//===----------------------------------------------------------------------===//<br>
+//<br>
+// Implementation of NclPopcountRecognize<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+NclPopcountRecognize::NclPopcountRecognize(LoopIdiomRecognize &TheLIR):<br>
+ LIR(TheLIR), CurLoop(TheLIR.getLoop()), PreCondBB(0) {<br>
+}<br>
+<br>
+bool NclPopcountRecognize::preliminaryScreen() {<br>
+ const ScalarTargetTransformInfo *STTI = LIR.getScalarTargetTransformInfo();<br>
+ if (STTI->getPopcntHwSupport(32) != ScalarTargetTransformInfo::Fast)<br></blockquote><div><br></div><div style>Why 32 here? This kind of magic number is worrisome.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
return false;<br>
<br>
- // Disable loop idiom recognition if the function's name is a common idiom.<br>
- StringRef Name = L->getHeader()->getParent()->getName();<br>
- if (Name == "memset" || Name == "memcpy")<br>
+ // Counting population are usually conducted by few arithmetic instrutions.<br>
+ // Such instructions can be easilly "absorbed" by vacant slots in a<br>
+ // non-compact loop. Therefore, recognizing popcount idiom only makes sense<br>
+ // in a compact loop.<br>
+<br>
+ // Give up if the loop has multiple blocks or multiple backedges.<br>
+ if (CurLoop->getNumBackEdges() != 1 || CurLoop->getNumBlocks() != 1)<br>
return false;<br>
<br>
- // The trip count of the loop must be analyzable.<br>
- SE = &getAnalysis<ScalarEvolution>();<br>
- if (!SE->hasLoopInvariantBackedgeTakenCount(L))<br>
+ BasicBlock *LoopBody = *(CurLoop->block_begin());<br>
+ if (LoopBody->size() >= 20) {<br></blockquote><div><br></div><div style>Why is 20 instructions too big? Shouldn't this be a ratio of instructions related to computing popcount and those not related? What if these instructions are all debug info intrinsics?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // The loop is too big, bail out.<br>
+ return false;<br>
+ }<br>
+<br>
+ // It should have a preheader containing nothing but a goto instruction.<br></blockquote><div><br></div><div style>We don't have goto instructions. Please try to use the IR terminology in comments in LLVM.</div><div style>
<br></div><div style>Also you don't explain why this is important for your transformation.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ BasicBlock *PreHead = CurLoop->getLoopPreheader();<br>
+ if (!PreHead || !LIRUtil::isAlmostEmpty(PreHead))<br>
+ return false;<br>
+<br>
+ // It should have a precondition block where the generated popcount instrinsic<br>
+ // function will be inserted.<br></blockquote><div><br></div><div style>What is the distinction between a precondition and a preheader? I really don't understand the placement logic here. The loop preheader is a BB that is *not* part of the loop, but dominates it (plus other constraints). IT sounds like the perfect place to insert this intrinsic. Do you mean the loop *header* here? If so, I think that what you are calling a 'precondition' is actually what LLVM's loop info calls a preheader.... But maybe I'm misunderstanding something.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ PreCondBB = LIRUtil::getPrecondBb(PreHead);<br>
+ if (!PreCondBB)<br>
+ return false;<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+Value *NclPopcountRecognize::matchCondition (BranchInst *Br,<br>
+ BasicBlock *LoopEntry) const {<br>
+ if (!Br || !Br->isConditional())<br>
+ return 0;<br>
+<br>
+ ICmpInst *Cond = dyn_cast<ICmpInst>(Br->getCondition());<br>
+ if (!Cond)<br>
+ return 0;<br>
+<br>
+ ConstantInt *CmpZero = dyn_cast<ConstantInt>(Cond->getOperand(1));<br>
+ if (!CmpZero || !CmpZero->isZero())<br>
+ return 0;<br>
+<br>
+ ICmpInst::Predicate Pred = Cond->getPredicate();<br>
+ if ((Pred == ICmpInst::ICMP_NE && Br->getSuccessor(0) == LoopEntry) ||<br>
+ (Pred == ICmpInst::ICMP_EQ && Br->getSuccessor(1) == LoopEntry))<br>
+ return Cond->getOperand(0);<br>
+<br>
+ return 0;<br>
+}<br>
+<br>
+bool NclPopcountRecognize::detectIdiom(Instruction *&CntInst,<br>
+ PHINode *&CntPhi,<br>
+ Value *&Var) const {<br>
+ // Following code tries to detect this idiom:<br>
+ //<br>
+ // if (x0 != 0)<br>
+ // goto loop-exit // the precondition of the loop<br>
+ // cnt0 = init-val;<br>
+ // do {<br>
+ // x1 = phi (x0, x2);<br>
+ // cnt1 = phi(cnt0, cnt2);<br>
+ //<br>
+ // cnt2 = cnt1 + 1;<br>
+ // ...<br>
+ // x2 = x1 & (x1 - 1);<br>
+ // ...<br>
+ // } while(x != 0);<br>
+ //<br>
+ // loop-exit:<br>
+ //<br>
+<br>
+ // step 1: Check to see if the look-back branch match this pattern:<br>
+ // "if (a!=0) goto loop-entry".<br>
+ BasicBlock *LoopEntry;<br>
+ Instruction *DefX2, *CountInst;<br>
+ Value *VarX1, *VarX0;<br>
+ PHINode *PhiX, *CountPhi;<br>
+<br>
+ DefX2 = CountInst = 0;<br>
+ VarX1 = VarX0 = 0;<br>
+ PhiX = CountPhi = 0;<br>
+ LoopEntry = *(CurLoop->block_begin());<br>
+<br>
+ // step 1: Check if the loop-back branch is in desirable form.<br></blockquote><div><br></div><div style>This is called the loop back-edge in the LoopInfo terminology. This is also the second step one comment, the first one only comenst variable declarations??? I'm confusied.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ {<br></blockquote><div><br></div><div style>Why do you have an extraneous scope here?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (Value *T = matchCondition (LIRUtil::getBranch(LoopEntry), LoopEntry))<br>
+ DefX2 = dyn_cast<Instruction>(T);<br>
+ else<br>
+ return false;<br></blockquote><div><br></div><div style>Instead of this, if the condition fails, and return false. Then set the value.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ // step 2: detect instructions corresponding to "x2 = x1 & (x1 - 1)"<br>
+ {<br>
+ if (DefX2->getOpcode() != Instruction::And)<br>
+ return false;<br>
+<br>
+ BinaryOperator *SubOneOp;<br>
+<br>
+ if ((SubOneOp = dyn_cast<BinaryOperator>(DefX2->getOperand(0))))<br>
+ VarX1 = DefX2->getOperand(1);<br>
+ else {<br>
+ VarX1 = DefX2->getOperand(0);<br>
+ SubOneOp = dyn_cast<BinaryOperator>(DefX2->getOperand(1));<br>
+ }<br>
+ if (!SubOneOp)<br>
+ return false; </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ Instruction *SubInst = cast<Instruction>(SubOneOp);<br>
+ ConstantInt *Dec = dyn_cast<ConstantInt>(SubInst->getOperand(1));<br>
+ if (!Dec ||<br>
+ !((SubInst->getOpcode() == Instruction::Sub && Dec->isOne()) ||<br>
+ (SubInst->getOpcode() == Instruction::Add && Dec->isAllOnesValue()))) {<br>
+ return false;<br>
+ }<br>
+ }<br></blockquote><div><br></div><div style>Please replace all of this code with the use of PatternMatch. See InstCombine or that header for details on how.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ // step 3: Check the recurrence of variable X<br>
+ {<br>
+ PhiX = dyn_cast<PHINode>(VarX1);<br>
+ if (!PhiX ||<br>
+ (PhiX->getOperand(0) != DefX2 && PhiX->getOperand(1) != DefX2)) {<br>
+ return false;<br>
+ }<br>
+ }<br></blockquote><div><br></div><div style>Similar here, although PatternMatch may not (yet) support PHI nodes. If not, it should be added.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ // step 4: Find the instruction which count the population: cnt2 = cnt1 + 1<br>
+ {<br>
+ CountInst = NULL;<br>
+ for (BasicBlock::iterator Iter = LoopEntry->getFirstNonPHI(),<br>
+ IterE = LoopEntry->end(); Iter != IterE; Iter++) {<br>
+ Instruction *Inst = Iter;<br>
+ if (Inst->getOpcode() != Instruction::Add)<br>
+ continue;<br>
+<br>
+ ConstantInt *Inc = dyn_cast<ConstantInt>(Inst->getOperand(1));<br>
+ if (!Inc || !Inc->isOne())<br>
+ continue;<br>
+<br>
+ PHINode *Phi = dyn_cast<PHINode>(Inst->getOperand(0));<br>
+ if (!Phi && Phi->getParent() != LoopEntry)<br></blockquote><div><br></div><div style>This && should be an || shouldn't it? Why isn't there a test case that covers this? Please add one as wel as fixing.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ continue;<br>
+<br>
+ // Check if the result of the instruction is live of the loop.<br>
+ bool LiveOutLoop = false;<br>
+ for (Value::use_iterator I = Inst->use_begin(), E = Inst->use_end();<br>
+ I != E; I++) {<br>
+ if ((cast<Instruction>(*I))->getParent() != LoopEntry) {<br>
+ LiveOutLoop = true; break;<br></blockquote><div style><br>Please don't place two statements on one line. *Especially* when one is a control flow statement!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br></blockquote><div><br></div><div style>Always hoist predicate loops like this into a helper function so that you can use 'return true'. in the loop. The coding standards talk about this.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (LiveOutLoop) {<br>
+ CountInst = Inst;<br>
+ CountPhi = Phi;<br>
+ break;<br>
+ }<br>
+ }<br></blockquote><div><br></div><div style>And this hoist this loop into a helper function so you can return the instructions found rather than having to test and set them here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (!CountInst)<br>
+ return false;<br>
+ }<br>
+<br>
+ // step 5: check if the precondition is in this form:<br>
+ // "if (x != 0) goto loop-head ; else goto somewhere-we-don't-care;"<br>
+ {<br>
+ BranchInst *PreCondBr = LIRUtil::getBranch(PreCondBB);<br>
+ Value *T = matchCondition (PreCondBr, CurLoop->getLoopPreheader());<br></blockquote><div><br></div><div style>Never have a space before an open parenthesis of a function call either. The conding standards discuss this.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (T != PhiX->getOperand(0) && T != PhiX->getOperand(1))<br>
+ return false;<br>
+<br>
+ CntInst = CountInst;<br>
+ CntPhi = CountPhi;<br></blockquote><div><br></div><div style>This is the third layer of storing an inner variable in an outer variable. These all need to abstracted into function calls.</div><div style><br></div><div style>
I know that there are three variables here, but output parameters and a bool return indicating success / failure will be immensely cleaner.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ Var = T;<br>
+ }<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+void NclPopcountRecognize::transform(Instruction *CntInst,<br>
+ PHINode *CntPhi, Value *Var) {<br>
+<br>
+ ScalarEvolution *SE = LIR.getScalarEvolution();<br>
+ TargetLibraryInfo *TLI = LIR.getTargetLibraryInfo();<br>
+ BasicBlock *PreHead = CurLoop->getLoopPreheader();<br>
+ BranchInst *PreCondBr = LIRUtil::getBranch(PreCondBB);<br>
+ const DebugLoc DL = CntInst->getDebugLoc();<br>
+<br>
+ // Assuming before transformation, the loop is following:<br>
+ // if (x) // the precondition<br>
+ // do { cnt++; x &= x - 1; } while(x);<br>
+<br>
+ // Step 1: Insert the ctpop instruction at the end of the precondition block<br>
+ IRBuilderTy Builder(PreCondBr);<br>
+ Value *PopCnt, *PopCntZext, *NewCount;<br>
+ {<br></blockquote><div style><br></div><div style>Again, please don't use {}s unless you are introducing a scoped variable of some kind.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ PopCnt = createPopcntIntrinsic(Builder, Var, DL);<br>
+ NewCount = PopCntZext =<br>
+ Builder.CreateZExtOrTrunc(PopCnt, cast<IntegerType>(CntPhi->getType()));<br>
+<br>
+ if (NewCount != PopCnt)<br>
+ (cast<Instruction>(NewCount))->setDebugLoc(DL);<br>
+<br>
+ // If the popoulation counter's initial value is not zero, insert Add Inst.<br>
+ Value *CntInitVal = CntPhi->getIncomingValueForBlock(PreHead);<br>
+ ConstantInt *InitConst = dyn_cast<ConstantInt>(CntInitVal);<br>
+ if (!InitConst || !InitConst->isZero()) {<br></blockquote><div><br></div><div style>This is incorrect, and will crash LLVM on certain inputs: if InitConst is null here, we pass the test and pass it to CreateAdd below that eventually segfaults. Please fix, and please add a test case for this case. I spotted it by inspection (after some code in the wild crashed). I can try to help you with a testcase if you need, but it should be quite straight forward to synthesize one.</div>
<div style><br></div><div style>You should try to always use one of the two patterns:</div><div style><br></div><div style>Foo *MyFooVaribale = getFoo();</div><div style>if (!MyFooVariable)</div><div style> return ...;</div>
<div style><br></div><div style>or</div><div style><br></div><div style>if (Foo *MyFooVariable = getFoo()) {</div><div style> ...</div><div style>}</div><div style><br></div><div style>The first handles early exit, the latter a predicated block. The reason this is good is that it makes it harder to write these types of bugs.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ NewCount = Builder.CreateAdd(PopCnt, InitConst);<br>
+ (cast<Instruction>(NewCount))->setDebugLoc(DL);<br></blockquote><div><br></div><div style>You don't need extra ()s around the cast.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ }<br>
+<br>
+ // Step 2: Replace the precondition from "if(x == 0) goto loop-exit" to<br>
+ // "if(NewCount == 0) loop-exit". Withtout this change, the intrinsic<br>
+ // function would be partial dead code, and downstream passes will drag<br>
+ // it back from the precondition block to the preheader.<br>
+ {<br>
+ ICmpInst *PreCond = cast<ICmpInst>(PreCondBr->getCondition());<br>
+<br>
+ Value *Opnd0 = PopCntZext;<br>
+ Value *Opnd1 = ConstantInt::get(PopCntZext->getType(), 0);<br>
+ if (PreCond->getOperand(0) != Var)<br>
+ std::swap(Opnd0, Opnd1);<br>
+<br>
+ ICmpInst *NewPreCond =<br>
+ cast<ICmpInst>(Builder.CreateICmp(PreCond->getPredicate(), Opnd0, Opnd1));<br>
+ PreCond->replaceAllUsesWith(NewPreCond);<br>
+<br>
+ deleteDeadInstruction(PreCond, *SE, TLI);<br>
+ }<br>
+<br>
+ // Step 3: Note that the population count is exactly the trip count of the<br>
+ // loop in question, which enble us to to convert the loop from noncountable<br>
+ // loop into a countable one. The benefit is twofold:<br>
+ //<br>
+ // - If the loop only counts population, the entire loop become dead after<br>
+ // the transformation. It is lots easier to prove a countable loop dead<br>
+ // than to prove a noncountable one. (In some C dialects, a infite loop<br>
+ // isn't dead even if it computes nothing useful. In general, DCE needs<br>
+ // to prove a noncountable loop finite before safely delete it.)<br>
+ //<br>
+ // - If the loop also performs something else, it remains alive.<br>
+ // Since it is transformed to countable form, it can be aggressively<br>
+ // optimized by some optimizations which are in general not applicable<br>
+ // to a noncountable loop.<br>
+ //<br>
+ // After this step, this loop (conceptually) would look like following:<br>
+ // newcnt = __builtin_ctpop(x);<br>
+ // t = newcnt;<br>
+ // if (x)<br>
+ // do { cnt++; x &= x-1; t--) } while (t > 0);<br>
+ BasicBlock *Body = *(CurLoop->block_begin());<br>
+ {<br>
+ BranchInst *LbBr = LIRUtil::getBranch(Body);<br>
+ ICmpInst *LbCond = cast<ICmpInst>(LbBr->getCondition());<br>
+ Type *Ty = NewCount->getType();<br>
+<br>
+ PHINode *TcPhi = PHINode::Create(Ty, 2, "tcphi", Body->begin());<br>
+<br>
+ Builder.SetInsertPoint(LbCond);<br>
+ Value *Opnd1 = cast<Value>(TcPhi);<br>
+ Value *Opnd2 = cast<Value>(ConstantInt::get(Ty, 1));<br>
+ Instruction *TcDec =<br>
+ cast<Instruction>(Builder.CreateSub(Opnd1, Opnd2, "tcdec", false, true));<br>
+<br>
+ TcPhi->addIncoming(NewCount, PreHead);<br>
+ TcPhi->addIncoming(TcDec, Body);<br>
+<br>
+ CmpInst::Predicate Pred = (LbBr->getSuccessor(0) == Body) ?<br>
+ CmpInst::ICMP_UGT : CmpInst::ICMP_SLE;<br>
+ LbCond->setPredicate(Pred);<br>
+ LbCond->setOperand(0, TcDec);<br>
+ LbCond->setOperand(1, cast<Value>(ConstantInt::get(Ty, 0)));<br>
+ }<br>
+<br>
+ // Step 4: All the references to the original population counter outside<br>
+ // the loop are replaced with the NewCount -- the value returned from<br>
+ // __builtin_ctpop().<br>
+ {<br>
+ SmallVector<Value *, 4> CntUses;<br>
+ for (Value::use_iterator I = CntInst->use_begin(), E = CntInst->use_end();<br>
+ I != E; I++) {<br>
+ if (cast<Instruction>(*I)->getParent() != Body)<br>
+ CntUses.push_back(*I);<br>
+ }<br>
+ for (unsigned Idx = 0; Idx < CntUses.size(); Idx++) {<br>
+ (cast<Instruction>(CntUses[Idx]))->replaceUsesOfWith(CntInst, NewCount);<br></blockquote><div><br></div><div style>No need for ()s around the cast.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br></blockquote><div><br></div><div style>Why the two loops and the vector? Why not just replace the uses as you find them?</div><div style><br></div><div style>Also, convention is to not use {}s for single line loops.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ // step 5: Forget the "non-computable" trip-count SCEV associated with the<br>
+ // loop. The loop would otherwise not be deleted even if it becomes empty.<br>
+ SE->forgetLoop(CurLoop);<br>
+}<br>
+<br>
+CallInst *NclPopcountRecognize::createPopcntIntrinsic(IRBuilderTy &IRBuilder,<br>
+ Value *Val, DebugLoc DL) {<br>
+ Value *Ops[] = { Val };<br>
+ Type *Tys[] = { Val->getType() };<br>
+<br>
+ Module *M = (*(CurLoop->block_begin()))->getParent()->getParent();<br>
+ Value *Func = Intrinsic::getDeclaration(M, Intrinsic::ctpop, Tys);<br>
+ CallInst *CI = IRBuilder.CreateCall(Func, Ops);<br>
+ CI->setDebugLoc(DL);<br>
+<br>
+ return CI;<br>
+}<br>
+<br>
+/// recognize - detect population count idiom in a non-countable loop. If<br>
+/// detected, transform the relevant code to popcount intrinsic function<br>
+/// call, and return true; otherwise, return false.<br>
+bool NclPopcountRecognize::recognize() {<br>
+<br>
+ if (!LIR.getScalarTargetTransformInfo())<br>
+ return false;<br>
+<br>
+ LIR.getScalarEvolution();<br>
+<br>
+ if (!preliminaryScreen())<br>
+ return false;<br>
+<br>
+ Instruction *CntInst;<br>
+ PHINode *CntPhi;<br>
+ Value *Val;<br>
+ if (!detectIdiom(CntInst, CntPhi, Val))<br>
return false;<br>
- const SCEV *BECount = SE->getBackedgeTakenCount(L);<br>
+<br>
+ transform(CntInst, CntPhi, Val);<br>
+ return true;<br>
+}<br>
+<br>
+//===----------------------------------------------------------------------===//<br>
+//<br>
+// Implementation of LoopIdiomRecognize<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+bool LoopIdiomRecognize::runOnCountableLoop() {<br>
+ const SCEV *BECount = SE->getBackedgeTakenCount(CurLoop);<br>
if (isa<SCEVCouldNotCompute>(BECount)) return false;<br>
<br>
// If this loop executes exactly one time, then it should be peeled, not<br>
@@ -199,24 +663,27 @@<br>
return false;<br>
<br>
// We require target data for now.<br>
- TD = getAnalysisIfAvailable<DataLayout>();<br>
- if (TD == 0) return false;<br>
+ if (!getDataLayout())<br>
+ return false;<br>
+<br>
+ getDominatorTree();<br>
<br>
- DT = &getAnalysis<DominatorTree>();<br>
LoopInfo &LI = getAnalysis<LoopInfo>();<br>
TLI = &getAnalysis<TargetLibraryInfo>();<br>
<br>
+ getTargetLibraryInfo();<br>
+<br>
SmallVector<BasicBlock*, 8> ExitBlocks;<br>
CurLoop->getUniqueExitBlocks(ExitBlocks);<br>
<br>
DEBUG(dbgs() << "loop-idiom Scanning: F["<br>
- << L->getHeader()->getParent()->getName()<br>
- << "] Loop %" << L->getHeader()->getName() << "\n");<br>
+ << CurLoop->getHeader()->getParent()->getName()<br>
+ << "] Loop %" << CurLoop->getHeader()->getName() << "\n");<br>
<br>
bool MadeChange = false;<br>
// Scan all the blocks in the loop that are not in subloops.<br>
- for (Loop::block_iterator BI = L->block_begin(), E = L->block_end(); BI != E;<br>
- ++BI) {<br>
+ for (Loop::block_iterator BI = CurLoop->block_begin(),<br>
+ E = CurLoop->block_end(); BI != E; ++BI) {<br>
// Ignore blocks in subloops.<br>
if (LI.getLoopFor(*BI) != CurLoop)<br>
continue;<br>
@@ -226,6 +693,33 @@<br>
return MadeChange;<br>
}<br>
<br>
+bool LoopIdiomRecognize::runOnNoncountableLoop() {<br>
+ NclPopcountRecognize Popcount(*this);<br>
+ if (Popcount.recognize())<br>
+ return true;<br>
+<br>
+ return false;<br>
+}<br>
+<br>
+bool LoopIdiomRecognize::runOnLoop(Loop *L, LPPassManager &LPM) {<br>
+ CurLoop = L;<br>
+<br>
+ // If the loop could not be converted to canonical form, it must have an<br>
+ // indirectbr in it, just give up.<br>
+ if (!L->getLoopPreheader())<br>
+ return false;<br>
+<br>
+ // Disable loop idiom recognition if the function's name is a common idiom.<br>
+ StringRef Name = L->getHeader()->getParent()->getName();<br>
+ if (Name == "memset" || Name == "memcpy")<br>
+ return false;<br>
+<br>
+ SE = &getAnalysis<ScalarEvolution>();<br>
+ if (SE->hasLoopInvariantBackedgeTakenCount(L))<br>
+ return runOnCountableLoop();<br>
+ return runOnNoncountableLoop();<br>
+}<br>
+<br>
/// runOnLoopBlock - Process the specified block, which lives in a counted loop<br>
/// with the specified backedge count. This block is known to be in the current<br>
/// loop and not in any subloops.<br>
<br>
Added: llvm/trunk/test/Transforms/LoopIdiom/popcnt.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopIdiom/popcnt.ll?rev=168931&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopIdiom/popcnt.ll?rev=168931&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/LoopIdiom/popcnt.ll (added)<br>
+++ llvm/trunk/test/Transforms/LoopIdiom/popcnt.ll Thu Nov 29 13:38:54 2012<br>
@@ -0,0 +1,76 @@<br>
+; RUN: opt -loop-idiom < %s -mtriple=x86_64-apple-darwin -mcpu=corei7 -S | FileCheck %s<br>
+<br>
+;To recognize this pattern:<br>
+;int popcount(unsigned long long a) {<br>
+; int c = 0;<br>
+; while (a) {<br>
+; c++;<br>
+; a &= a - 1;<br>
+; }<br>
+; return c;<br>
+;}<br>
+;<br>
+; CHECK: entry<br>
+; CHECK: llvm.ctpop.i64<br>
+; CHECK: ret<br>
+define i32 @popcount(i64 %a) nounwind uwtable readnone ssp {<br>
+entry:<br>
+ %tobool3 = icmp eq i64 %a, 0<br>
+ br i1 %tobool3, label %while.end, label %while.body<br>
+<br>
+while.body: ; preds = %entry, %while.body<br>
+ %c.05 = phi i32 [ %inc, %while.body ], [ 0, %entry ]<br>
+ %a.addr.04 = phi i64 [ %and, %while.body ], [ %a, %entry ]<br>
+ %inc = add nsw i32 %c.05, 1<br>
+ %sub = add i64 %a.addr.04, -1<br>
+ %and = and i64 %sub, %a.addr.04<br>
+ %tobool = icmp eq i64 %and, 0<br>
+ br i1 %tobool, label %while.end, label %while.body<br>
+<br>
+while.end: ; preds = %while.body, %entry<br>
+ %c.0.lcssa = phi i32 [ 0, %entry ], [ %inc, %while.body ]<br>
+ ret i32 %c.0.lcssa<br>
+}<br>
+<br>
+; To recognize this pattern:<br>
+;int popcount(unsigned long long a, int mydata1, int mydata2) {<br>
+; int c = 0;<br>
+; while (a) {<br>
+; c++;<br>
+; a &= a - 1;<br>
+; mydata1 *= c;<br>
+; mydata2 *= (int)a;<br>
+; }<br>
+; return c + mydata1 + mydata2;<br>
+;}<br>
+; CHECK: entry<br>
+; CHECK: llvm.ctpop.i64<br>
+; CHECK: ret<br>
+define i32 @popcount2(i64 %a, i32 %mydata1, i32 %mydata2) nounwind uwtable readnone ssp {<br>
+entry:<br>
+ %tobool9 = icmp eq i64 %a, 0<br>
+ br i1 %tobool9, label %while.end, label %while.body<br>
+<br>
+while.body: ; preds = %entry, %while.body<br>
+ %c.013 = phi i32 [ %inc, %while.body ], [ 0, %entry ]<br>
+ %mydata2.addr.012 = phi i32 [ %mul1, %while.body ], [ %mydata2, %entry ]<br>
+ %mydata1.addr.011 = phi i32 [ %mul, %while.body ], [ %mydata1, %entry ]<br>
+ %a.addr.010 = phi i64 [ %and, %while.body ], [ %a, %entry ]<br>
+ %inc = add nsw i32 %c.013, 1<br>
+ %sub = add i64 %a.addr.010, -1<br>
+ %and = and i64 %sub, %a.addr.010<br>
+ %mul = mul nsw i32 %inc, %mydata1.addr.011<br>
+ %conv = trunc i64 %and to i32<br>
+ %mul1 = mul nsw i32 %conv, %mydata2.addr.012<br>
+ %tobool = icmp eq i64 %and, 0<br>
+ br i1 %tobool, label %while.end, label %while.body<br>
+<br>
+while.end: ; preds = %while.body, %entry<br>
+ %c.0.lcssa = phi i32 [ 0, %entry ], [ %inc, %while.body ]<br>
+ %mydata2.addr.0.lcssa = phi i32 [ %mydata2, %entry ], [ %mul1, %while.body ]<br>
+ %mydata1.addr.0.lcssa = phi i32 [ %mydata1, %entry ], [ %mul, %while.body ]<br>
+ %add = add i32 %mydata2.addr.0.lcssa, %mydata1.addr.0.lcssa<br>
+ %add2 = add i32 %add, %c.0.lcssa<br>
+ ret i32 %add2<br>
+}<br>
+<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>