[patch] simplifyCFG for review

[Andrew Trick]

On Jul 27, 2013, at 12:08 AM, Chandler Carruth <chandlerc at google.com> wrote:

> 
> On Fri, Jul 26, 2013 at 11:18 PM, Chandler Carruth <chandlerc at google.com> wrote:
> #3 is *definitely* wrong. Please revert that part immediately.
> 
> OK, I've investigated this more closely, and it turns out to be benign on any target but R600, so *when* it comes out doesn't really matter, but it definitely isn't right.
> 
> When I investigated it, I found this part of the entire patch really strange. We have had for some time the questionable decision of passing TargetTransformInfo down into SimplifyCFG and using it. One might think that 'IsTargetAware' controls this decision. But it doesn't. Instead, it controls whether we pass a non-null AliasAnalysis to SimplifyCFG, despite the fact that AA is a totally target independent thing and would be reasonable in the canonicalizing form of SimplifyCFG. This interface speaks to problem #2 I mentioned in my prior email.
> 
> Now, as it happens, the only thing that uses AA in SimplifyCFG is the newly added logic such that this has the intended effect, but I don't think this mechanism makes much sense.
> 
> Making TTI not be used in the canonicalizing phase is part of the larger refactoring of the midlevel optimizer that Andy's email aluded to and that I think is really important to happen before we start adding lots of target-specific logic to passes that share logic between the phases. We need to get interfaces for those *right*. However, I think a specialized pass that is added late in the pipeline by the target continues to be a reasonable compromise to allow forward progress until that refactoring happens.

Ok, I hear you Chandler. And I know you read my initial objections.

There's no need to revert because the patch only affects one target and really isn't invasive, it's just not well organized yet.

I ok'd this on the grounds of minimal acceptability. In the past, I would have insisted that this be a new pass activated with the addPreISel hook. But I've been wanting to split SimplifyCFG into two passes anyway--this patch makes a bit of progress in that direction.

I'm unconvinced there's a real advantage in interleaving the new CFG flattening stuff with the SimplifyCFG transforms, but it's something we may want to support in general.

I agree that optimizecfg should not be inside CGSCC, but neither should some other passes. I thought that could wait until the proposed pass reorganization. I also agree that the canonical SimplifyCFG should not require TTI, but that's clearly an independent problem. Yes, there is plenty of strangeness about the current design--I imagine fixing it one patch at a time.

There are at least two things that should be fixed very soon.

(1) In terms of code organization, these anti-canonical, target-selected transforms should live somewhere else. I kept quiet becase I hadn't come up with an alternative, but we can do that now. OptimizeCFG.cpp?

(2) Controlling the target-specific opts. Using the existence of AA is just wrong. Apologies for letting that go. The simplest thing to do is have an OptimizedCFG(BB) call immediately after SimplifyCFG(BB) gated on isTargetAware. I can't remember if Mei's code requires utilities in the generic code. If so, then we just need another header in include/llvm/Transforms/Utils.

Do you have any problem with this all living in libLLVMTransformUtils?

-Andy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130727/f5aa5447/attachment.html>