[PATCH] Rename determineInsertionPoints to determinePHINodes, expose it so it can be reused

[Quentin Colombet]

Hi Daniel,

> On Apr 20, 2015, at 3:21 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> 
> On Mon, Apr 20, 2015 at 3:07 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>> 
>>> On Apr 20, 2015, at 2:44 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>> 
>>> On Mon, Apr 20, 2015 at 2:29 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> Hi Daniel
>>>> 
>>>> 
>>>> ================
>>>> Comment at: include/llvm/Transforms/Utils/PromoteMemToReg.h:70
>>>> @@ +69,3 @@
>>>> +                       const DenseMap<DomTreeNode *, unsigned> &DomLevels,
>>>> +                       SmallVectorImpl<std::pair<unsigned, BasicBlock *>> &PHIBlocks);
>>>> +
>>>> ----------------
>>>> From the user stand point I think we should not have to provide the DomLevels and the LiveInBlocks.
>>> 
>>> The only way to do this would be callback functions, for two reasons:
>>> 
>>> 1 .The live-in-blocks require knowledge of what a def and a use is, at
>>> the very least.
>>> For these two use cases, for memory ssa, this is not the same as ssa
>>> (for MemorySSA, because it's not part of the IR, so we'd have to
>>> provide a way to walk it)
>>> 
>>> For PromoteMemToReg, it wants live-in blocks for specific alloca's
>>> passed to PromoteMemToReg (in particular, the one is it currently
>>> determining phi's for, since it does it per-alloca).
>>> 
>>> This also would require a callback for this function to determine.
>> 
>> Well, what about providing the list of uses then?
> 
> If it wanted to compute it from scratch, it would need the list of
> uses and defs it is caring about,

That is what I meant sorry.
A list of all the uses and defs related to the “variable” we want to SSAify. And I mean instructions, not block. We can drop the blocks.

> the basic blocks they belong to,

getParent(), no need for extra info ;).

> and
> their relative positions (so it knows if def happens before use in a
> given block).

This can be computed: is InstA dominates InstB. Currently I believe this check is expensive, but that would be a good use case of improving that :).

>  Otherwise, it would mark things that are defined before
> used in a block as live-in when they are not.
> 
> This would be O(the size of the entire function) extra storage :)
> 
> It could also be done by passing in DefBeforeUse blocks and
> UsingBlocks, but at least to me, it seems less user friendly that just
> giving the set of live-in blocks.

Agree on that aspect, thus the list of instructions based API.

> 
> Happy to do it though if you disagree.


I think the list of def / use is easier to get from the user perspective and let the algo handles all the gory details.

What about giving more granularity in the API?

We could expose a DT, Defs<Inst>, Uses<Inst>, like API.
But also the DT, Defs<Block>, LiveInBlock, API with some helpers to compute the different information, like LiveInBlock, DomLevel etc.

We can also envision a version that actually updates the uses to the proper inserted PHIs.

I may go to far in the refactoring as maybe I missed the short term goal.

The bottom line is I do not want to hold on the commit if it suits whatever the short term usages are, if other people agrees. We can rework that as the new users are exposed :).

Cheers,
-Quentin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150420/98b8a2e2/attachment.html>