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

Quentin Colombet qcolombet at apple.com
Mon Apr 20 15:07:09 PDT 2015


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

> 
> 2.   DomLevels does not change per-variable, and so, both memoryssa
> and promotememtoreg only compute it once per function.
> 
> If we compute it ourselves here, we will compute it once per variable.

Right, but this information is already available in the DominatorTree argument so it feels weird to have the users having to provide this information. Like you said, we can wrap this thing in something :).

> 
> 
> I could wrap this whole thing in a class to solve the second problem.
> It would not solve the first.
> 
> 
> 
> 
>> 
>> The BBNumbers are already on your “to-remove” list, so it is fine for now.
>> 
>> http://reviews.llvm.org/D9118
>> 
>> EMAIL PREFERENCES
>>  http://reviews.llvm.org/settings/panel/emailpreferences/
>> 
>> 





More information about the llvm-commits mailing list