[PATCH] Remove CloneFunction VMap

[Pete Cooper]

> On Apr 28, 2015, at 2:23 PM, Tobias Edler von Koch <tobias at codeaurora.org> wrote:
> 
> Hi Pete,
> 
> On Tue, 28 Apr 2015 13:21:19 -0700 Pete Cooper <peter_cooper at apple.com>
> wrote:
> 
>>> I have a downstream pass using this, but it's not a big deal because
>>> it's possible to replicate the old behavior by creating a new
>>> function yourself and then using CloneFunctionInto, which lets you remap
>>> the arguments.
>> So it took a look at the callers of CloneFunctionInto and none of them are using this functionality either.  I’d love to remove all of the attribute set hacking at the top of CloneFunctionInto, but that’ll break your out-of-tree code.  Specifically, its this code, and the loop under it:
>> 
>>  // Copy all attributes other than those stored in the AttributeSet.  We need
>>  // to remap the parameter indices of the AttributeSet.
>>  AttributeSet NewAttrs = NewFunc->getAttributes();
>>  NewFunc->copyAttributesFrom(OldFunc);
>>  NewFunc->setAttributes(NewAttrs);
>> 
>> If this code is particularly useful to you then i’m happy to leave it as is.  Otherwise, if there’s some way I can delete the unused code in CloneFunctionInto then that would be great.
> 
> I agree this code shouldn't really be in CloneFunctionInto because it
> doesn't clone (what I would consider) 'content' of the function, but
> rather 'meta-information'/attributes.
> 
> My use case is about substituting constants for function arguments and
> as far as I can see that's still possible by putting them into the VMap
> before calling CloneFunctionInto, right? The copying of attributes can
> be done manually.
Thats a good use case.  I was actually very surprised to find that there’s nothing similar in-tree.  I suspect there probably is, but perhaps its a combination of DeadArgElimination and SCCP.  As an alternative to removing this code, here’s what I currently have locally, which I think is a bit cleaner:

  // Copy all attributes other than those stored in the AttributeSet.  We need
  // to remap the parameter indices of the AttributeSet.
  NewFunc->copyAttributesFrom(OldFunc);

  SmallVector<AttributeSetRef, 8> AttributesVec;
  AttributeSet OldAttrs = OldFunc->getAttributes();

  // Add any return attributes.
  if (auto Attrs = OldAttrs.getRetAttributes())
    AttributesVec.push_back(Attrs);

  // Clone any argument attributes that are present in the VMap.
  for (const Argument &OldArg : OldFunc->args())
    if (Argument *NewArg = dyn_cast<Argument>(VMap[&OldArg])) {
      if (auto Attrs = OldAttrs.getParamAttributes(OldArg.getArgNo() + 1))
        AttributesVec.push_back(Attrs.setDest(1 + NewArg->getArgNo()));
    }

  // Add any function attributes.
  if (auto Attrs = OldAttrs.getFnAttributes())
    AttributesVec.push_back(Attrs);

  NewFunc->setAttributes(AttributeSet::get(NewFunc->getContext(),
                                           AttributesVec));
> 
> I wonder whether there's anything we can do to make sure out-of-tree
> stuff doesn't silently and subtly break when you make this change.
> Change the function name? Change the argument list?
Good question.  I could certainly change the code to assert that arguments aren’t in the VMap at all.  Then its going to crash immediately for any out-of-tree users, instead of being subtly different behavior.  Unfortunately i think CloneModule actually makes use of the VMap argument, so changing the signature to remove that argument for example wouldn’t work.  I like the idea though, and will see if any other arguments are perhaps unused.
> 
> Just out of curiosity - are you trying to remove dead code across the
> code base or what's the motivation for your work?
I’ve been looking to clean up some of the uses of AttributeSets.  I noticed that many places create temporary AttributeSets and don’t need to.  I came across CloneFunction due to going through all the callers of getFnAttributes and noticed that the code wasn’t really required.  The first of my potential AttributeSet commits it out for review here: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/272820.html <http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/272820.html>

As I said in that email, i’m happy for suggestions on AttributeSets if anyone has seen anything else which could be improved.  I’m going through all the callers of all the methods to see what can be refactored.  I’m first trying to remove temporaries, but now also looking to hide some of the internals.  No-one should need to know that an AttributeSet has slots for example.

Cheers,
Pete
> 
> Thanks,
> Tobias
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150428/71be0e82/attachment.html>