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

[James Molloy]

Hi Chris,

Thanks for the review. Replying now instead of with an updated patch as
I won't be able to get around to it until Monday.

On Thu, 2012-12-13 at 23:35 +0000, Chris Lattner wrote:
> 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.

Sure, will do.

>
> @@ -185,6 +189,8 @@
>    if (isa<IndirectBrInst>(BB->getTerminator()))
>      containsIndirectBr = true;
>
> +  notDuplicatable |= containsIndirectBr;
>
> is containsIndirectBr useful independently, or can it just be removed?

Good question; I'll investigate this further.

>
>
>
> +++ 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.

Sure.

>
>
>
> +        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.

I can try. It all comes down to the reading of the spec and the spirit
of the spec, and "ambiguous" is a kind way to describe it...

Michael Kuperstein in his review of this code put it reasonably
concisely:

https://groups.google.com/d/msg/llvm-dev/DSXnLD3j-Bg/CF15KI65od8J

It comes down to our reading of the spec (and being enforced by some
devices in practice) that the equivalence relation over barrier() calls
is keyed not just on the barrier's location in the source but the call
path used to reach it.

So:

void a() { barrier(); }
void b(int x) {
  if (x)
    a();
  else
    a();
}

The two calls to barrier here are *different*, not the same. That in
turn gave rise to the fully transitive semantic.

I agree that it is not in any way intuitive. I agree that it is likely
to fall over in the case of devirtualization, and in many ways it'll
fall over if hit by a stiff breeze. I think this is fallout from a very
poor spec that describes the behaviour it is trying to model. If you
have *any* suggestions about how to make it better, please let me
know! :)

>
>
> +++ 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?

I'll look into this.

>
>
>
> +++ 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?
>

Sure; I wasn't 100% happy with the wording anyway.

>
> +    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.

See my previous comment about not being happy with the wording :(

I'll go back and try and explain it better.

>
> +    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.

Yes, I agree completely. noduplicate models part of the barrier()
intrinsic that is not currently modellable in LLVM.

>   Independent of that, this language shouldn't be in LangRef.

OK, I'll remove it.

Part of the problem (with this entire patch) is that this is a behaviour
that will only work in certain situations - it's likely to fall down
with exception handling, devirtualization, and a load of other things
that luckily* OpenCL doesn't support. Unfortunately that does mean we've
got to invent semantics for the behaviour that make sense when applied
to stuff that isn't in OpenCL (such as devirt).

>
> -Chris

Cheers,

James

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.