[PATCH] Remove CloneFunction VMap

Tobias Edler von Koch tobias at codeaurora.org
Tue Apr 28 15:03:30 PDT 2015


On Tue, 28 Apr 2015 14:34:41 -0700 Pete Cooper <peter_cooper at apple.com>
wrote:

> > 
> > 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);
>[...]

Yeah that's a bit cleaner, but like I said I wouldn't mind too much if
it's removed altogether if it's not needed elsewhere in tree.

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

Well, asserting on arguments in the VMap won't work because it breaks
the use case I described above (you'd want to have an Argument ->
Constant mapping in the VMap).

Perhaps renaming is the best idea - how about CloneFunctionBodyInto? I
think that's really what it does. 

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

That makes sense!

Tobias

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project.




More information about the llvm-commits mailing list