patch for inlining all blocks with address taken

[Eric Christopher]

OK, apparently a testcase isn't easily possible because it has a
prerequisite on being able to inline the blocks from the patch that
was reverted.

That said, I guess having a time bomb in the code isn't such a good
thing since with better escape analysis we might be able to inline it.

Some nits on the patch:

+    /// CloneBlock - The specified block is found to be reachable, so clone it
+    /// into newBB.
     void CloneBlock(const BasicBlock *BB,
-                    std::vector<const BasicBlock*> &ToClone);
+                    BasicBlock *NewBB,
+                    std::vector<const BasicBlock *> &ToClone,
+                    std::set<const BasicBlock *> &OrigBBs);

Should probably document the point of the new arguments.

+/// CloneBlock - The specified block is found to be reachable, so clone it
+/// into newBB.
 void PruningFunctionCloner::CloneBlock(const BasicBlock *BB,

Ditto.

+  // This list will be pruned down by the CloneFunction() currently
+  // (March 2014) due to two optimizations:

Remove the time/date/currently part.

+    // It is only legal to clone a function if a block address within that
+    // function is never referenced outside of the function.  Given that, we

Probably want to add the comment that we should only get to that point
if the addresses aren't reachable from the outside. There's no way to
add an assert for this condition, but at least documenting it would be
good.

+  // Removed BB's that were created that turned out to be prunable.

s/Removed/Remove

+  // Alternatively we could write a routine CloneFunction and add a) a
+  // parameter to actually do the cloning and b) a return parameter that
+  // gives a list of blocks that need to be cloned also. Then we could
+  // call CloneFunction when we collect the blocks to call, but suppress
+  // cloning. And actually *do* the cloning in the while loop above. Also
+  // the cleanup here would become redundant, and so would be the OrigBBs.

Seems like a FIXME?

Otherwise the logic seems to work.

Thanks.

-eric


On Wed, Apr 23, 2014 at 5:00 PM, Eric Christopher <echristo at gmail.com> wrote:
> A testcase should be easily possible and I'd like to see one before we
> approve it :)
>
> -eric
>
> On Wed, Apr 23, 2014 at 12:42 PM, Manman Ren <manman.ren at gmail.com> wrote:
>> Hi Gerolf,
>>
>> The implementation looks good to me, but the testing case will not work
>> since r206429 was reverted.
>> If it is not possible to come up with a testing case, please try your best
>> to explain that in the commit log.
>>
>> Thanks,
>> Manman
>>
>>
>>
>> On Thu, Apr 17, 2014 at 2:23 PM, Gerolf Hoflehner <ghoflehner at apple.com>
>> wrote:
>>>
>>> any opinions?
>>>
>>> On Apr 14, 2014, at 3:29 PM, Gerolf Hoflehner <ghoflehner at apple.com>
>>> wrote:
>>>
>>> > Hi
>>> >
>>> > In some cases a reference to a cloned block is not available since it
>>> > has not been cloned yet. The net effect is that some block may not get
>>> > inlined. The attached patch solves this timing problem by first collecting
>>> > all candidate inline blocks and computing the references as necessary, and
>>> > then proceeding with the actual cloning.
>>> >
>>> > Gerolf
>>> >
>>> > <r16427209.patch>_______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at cs.uiuc.edu
>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>