[llvm-commits] [LLVMdev] [RFC] "noclone" function attribute

[Chris Lattner]

On Dec 19, 2012, at 7:56 AM, James Molloy <James.Molloy at arm.com> wrote:
> Hi Chris,
> 
> After a 1-day haitus, attaching a new patch.

This patch looks good to me, and I'm going out on vacation tomorrow, so I'll approve it with a few changes:

1) The testcases (which are great!) can probably be merged into other existing .ll files.  Please do, just to make the tests more granular.

2) The answer for CloneBasicBlock is non-obvious.  I'd prefer that you discuss it with others on this list (maybe Nadav has some advice here) and come to an agreement over what the right thing to do is, before changing it.  Yes, this means that noduplicate will be violated by this codepath, but I'd rather stage things. 

I see two possible approaches here: 1) make this the clients problem to deal with: they shouldn't call CloneBasicBlock unless it is safe.  2) go with your patch, and accept the compile-time performance impact.  I wonder if #1 is feasible, if so, it seems best.  The inliner (for example) already does the checking required.

> If you'd prefer I could add those to CodeMetrics so the whole thing
> could be ported over?

This would be option #3, I don't know the tradeoff here, but it doesn't seem as elegant as #1.


3) In Loop::isSafeToClone(), you don't need to check for InvokeInst in the inner loop of the basic block scan.  Just check for InvokeInst where IndirectBrInst is checked, since it is a terminator.

4) I don't see any bitcode reader/writer logic.  Please make sure these are persisted properly in bc files.

-Chris