[PATCH] Templatify RegionInfo

Matt Arsenault Matthew.Arsenault at amd.com
Wed Jul 9 18:19:47 PDT 2014


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.

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







More information about the llvm-commits mailing list