[PATCH] Templatify RegionInfo

Tobias Grosser tobias at grosser.es
Wed Jul 9 23:56:13 PDT 2014


On 10/07/2014 03:19, Matt Arsenault wrote:
> On 07/09/2014 12:05 AM, Tobias Grosser wrote:
>> On 09/07/2014 06:35, Matt Arsenault wrote:
>>>
>>> On Jul 8, 2014, at 4:06 PM, Tobias Grosser <tobias at grosser.es> wrote:
>>>
>>>> On 08/07/2014 21:37, Chandler Carruth wrote:
>>>>> So, I'm really fine with experiments going on in tree here, I just
>>>>> wanted
>>>>> to know what the plan was (which Matt's email pretty much
>>>>> clarified) and
>>>>> make sure that there wasn't going to be increased confusion (which a
>>>>> comment such as Tobias mentions would do).
>>>>>
>>>>> I don't think any of what has been discussed merits things being
>>>>> out-of-tree.
>>>>
>>>> OK.
>>>>
>>>> Matt, I just tried your patches they look rather mechanical so I
>>>> have little concerns. However, when testing I saw that they did
>>>> break the Polly build in several ways. Is this expected? You don't
>>>> happen to have a patch for Polly available, no?
>>>>
>>>> Cheers,
>>>> Tobias
>>>
>>> Here is the patch to fix the polly build. Most of the problems were
>>> from missing includes that RegionInfo.h used to include, and renaming
>>> of the pass to RegionInfoPass.
>>
>> I wonder if this renaming is necessary. Looking at DominanceFrontier
>> some kind of proxy pattern has been used to make the original functions
>> available in the DF pass.
> I think it's a good idea to split the actual working part from the pass,
> like what DominatorTree does. As far as what DominanceFrontier is doing,
> I thought it looked dumb at first. It's what
> DominatorTree/PostDominatorTree do, and I understood why at one point,
> but I've forgotten that already.

It seems PostDominatorTree also added functions into the
PostDominatorClass that forward the requests:

struct PostDominatorTree : public FunctionPass {
   PostDominatorTree() : FunctionPass(ID) {
     initializePostDominatorTreePass(*PassRegistry::getPassRegistry());
     DT = new DominatorTreeBase<BasicBlock>(true);
   }

   inline const std::vector<BasicBlock*> &getRoots() const {
     return DT->getRoots();
   }

   inline DomTreeNode *getRootNode() const {
     return DT->getRootNode();
   }
}

The working part is still split into the DominatorTreeBase<>() class,
but the interface is propagated to the pass. I think it would be nice to
do the same, but won't insist on it if you don't think its useful.
Overall, the changes in Polly are rather small.

>>> There’s another unfortunate
>>> reinterpret_cast since Region is no longer simply a subclass of
>>> RegionNode (if there’s a better solution to this that would be nice)
>>
>> Does just typedef-ing RegionNode to
>> RegionNodeBase<RegionTraits<Function>> not work? Alternatively, maybe
>> some automatically converting constructors (not fully sure how to write
>> them)?
>>
>> Cheers,
>> Tobias
>>
> The Region / RegionNode subclasses at first I created because some
> places were forward declaring them, and you can't do that with a
> typedef. They're also used to hide a few more reinterpret_casts,
> although it might work if I remove all the forward declares.

I see. I think the current solution is fine as well.

 From my side the code looks good.

Cheers,
Tobiaa



More information about the llvm-commits mailing list