Hi, while reading the TailDuplication pass, I found a check that looks rather pointless. TailDuplication looks at an unconditional branch instruction, BI. It performs a number of checks on the successor of this branch instruction, Dest. One of this checks is counting the number of predecessor. If this count is zero, Dest is regarded as dead and no tail duplication happens. However, as far as I can see, there is no way that Dest can have zero predecessors. By definition, it has at least one: The block containing BI. Is there a point I am missing here, or is this really a useless check? In particular, I'm talking about the following piece of code from lib/Transforms/Scalar/TailDuplication.cpp: pred_iterator PI = pred_begin(Dest), PE = pred_end(Dest); if (PI == PE && Dest != Dest->getParent()->begin()) return false; // It's just a dead block, ignore it... The attached patch removes this check. I can't find any problems with it, it causes no tests to fail. Gr. Matthijs -------------- next part -------------- A non-text attachment was scrubbed... Name: taildup.diff Type: text/x-diff Size: 865 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080514/210bb1ad/attachment.diff> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: Digital signature URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080514/210bb1ad/attachment.sig>
On May 14, 2008, at 7:26 AM, Matthijs Kooijman wrote:> Hi, > > while reading the TailDuplication pass, I found a check that looks > rather > pointless. > > TailDuplication looks at an unconditional branch instruction, BI. It > performs > a number of checks on the successor of this branch instruction, > Dest. One of > this checks is counting the number of predecessor. If this count is > zero, Dest > is regarded as dead and no tail duplication happens. > > However, as far as I can see, there is no way that Dest can have zero > predecessors. By definition, it has at least one: The block > containing BI. Is > there a point I am missing here, or is this really a useless check? > > In particular, I'm talking about the following piece of code from > lib/Transforms/Scalar/TailDuplication.cpp: > > pred_iterator PI = pred_begin(Dest), PE = pred_end(Dest); > if (PI == PE && Dest != Dest->getParent()->begin()) > return false; // It's just a dead block, ignore it... > > The attached patch removes this check. I can't find any problems > with it, it > causes no tests to fail.Looks good. Committed revision 51154.> > > Gr. > > Matthijs > <taildup.diff>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev- Devang