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

[Chris Lattner]

On Dec 13, 2012, at 3:18 AM, James Molloy <James.Molloy at arm.com> wrote:

> Pinging for review!

It's going in the right direction.  Some details:

This:

 bool Loop::isSafeToClone() const {
+    for (BasicBlock::iterator BI = (*I)->begin(), BE = (*I)->end(); BI != BE; ++BI) {
+      if (const CallInst *CI = dyn_cast<CallInst>(BI)) {
+        if (CI->hasFnAttr(Attributes::NoDuplicate))
+          return false;

and:

+++ lib/Analysis/CodeMetrics.cpp	(working copy)
@@ -165,6 +165,10 @@
     if (isa<ExtractElementInst>(II) || II->getType()->isVectorTy())
       ++NumVectorInsts;
 
+    if (const CallInst *CI = dyn_cast<CallInst>(II))
+      if (CI->hasFnAttr(Attributes::NoDuplicate))
+        notDuplicatable = true;

Should both handle "invoke" instructions marked "noduplicate" as well.

@@ -185,6 +189,8 @@
   if (isa<IndirectBrInst>(BB->getTerminator()))
     containsIndirectBr = true;
 
+  notDuplicatable |= containsIndirectBr;

is containsIndirectBr useful independently, or can it just be removed?



+++ lib/VMCore/Verifier.cpp	(working copy)
@@ -695,6 +695,25 @@
   // Check function attributes.
   VerifyFunctionAttrs(FT, Attrs, &F);
 
+  // The noduplicate attribute is transitive.
+  if (F.cannotDuplicate()) {
+    std::set<Function*> Fns;

If this stays, please use SmallPtrSet instead of std::set.



+        Assert1((*I)->cannotDuplicate(), "All functions which may transitively call a "
+                "noduplicate function must themselves be noduplicate!", &F);

This doesn't make a lot of sense to me to enforce.  Can you explain the intuition for this limitation?  In practice, this will be difficult to handle, because devirtualization (and other things) can turn an indirect call to a direct call… if that direct call has the wrong noduplicate sense, we will get bad things happening.


+++ lib/Transforms/Utils/CloneFunction.cpp	(working copy)
@@ -46,6 +46,14 @@
   // Loop over all instructions, and copy them over.
   for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
        II != IE; ++II) {
+    // It is illegal to clone a noduplicate call into the same parent function.
+    // If F is NULL or not BB->getParent(), we can't tell whether this is legal or
+    // not so we won't assert here.
+    if (const CallInst *CI = dyn_cast<CallInst>(II)) {
+      assert((F != BB->getParent() || !CI->hasFnAttr(Attributes::NoDuplicate)) &&
+             "Cannot clone a call marked 'noduplicate' into the same function!");

this should switch to using CodeMetrics, no?



+++ include/llvm/Analysis/CodeMetrics.h	(working copy)
@@ -46,9 +46,15 @@
     /// \brief True if this function calls itself.
     bool isRecursive;
 
-    /// \brief True if this function contains one or more indirect branches.
+    /// \brief True if this fucntion contains one or more indirect branches.
     bool containsIndirectBr;
 
+    /// \brief True if this function cannot be duplicated.
+    ///
+    /// True if this function contains one or more indirect branches, or it contains
+    /// one or more 'noduplicate' instructions.
+    bool notDuplicatable;


Is it useful to keep around containsIndirectBr?  Are there any clients that will use containsIndirectBr but not notDuplicatable?


+++ docs/LangRef.rst	(working copy)

+    duplicated. A call to a ``noduplicate`` function may be moved
+    within its parent function, but may not be duplicated within
+    its parent function.

instead of "parent function" how about "callee", or just "cannot be duplicated" or "must be emitted as a single call assembly level call" or something like that?


+    The ``noduplicate`` attribute is transitive. If function A
+    calls B which calls C, and the B -> C call is ``noduplicate``,
+    it is illegal for the optimizer to duplicate the A -> B call.

This still doesn't make sense to me at all.

+    The noduplicate attribute is intended to model the behaviour of
+    OpenCL's ``barrier()`` intrinsic.

I still think that noduplicate is part of barrier() but is probably not sufficient.  Independent of that, this language shouldn't be in LangRef.

-Chris