[PATCH] [OPENMP] llvm.loop.vectorize.enable metadata are lost after critical edge splitting

[Hal Finkel]

----- Original Message -----
> From: "Andrew Trick" <atrick at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D5539+public+7680b8f2f557d7c9 at reviews.llvm.org, "llvm commits" <llvm-commits at cs.uiuc.edu>, "zinovy nis"
> <zinovy.nis at gmail.com>, chandlerc at gmail.com
> Sent: Tuesday, September 30, 2014 4:47:48 PM
> Subject: Re: [PATCH] [OPENMP] llvm.loop.vectorize.enable metadata are lost after critical edge splitting
> 
> 
> On Sep 30, 2014, at 2:37 PM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> ----- Original Message -----
> 
> 
> From: "Andrew Trick" < atrick at apple.com >
> To: "zinovy nis" < zinovy.nis at gmail.com >, chandlerc at gmail.com ,
> atrick at apple.com
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Tuesday, September 30, 2014 4:29:08 PM
> Subject: Re: [PATCH] [OPENMP] llvm.loop.vectorize.enable metadata are
> lost after critical edge splitting
> 
> Is it incorrect to leave the loop metadata on the original branch
> when splitting the edge?
> 
> GVN already requires DomTree, so no change to the pipeline is needed
> if you use it.
> 
> In splitCriticalEdges, if LoopInfo is available you can use it.
> Otherwise you can use the DomTree. If the DomTree check fails (ie.
> both targets dominate the latch) then just invalidate the metadata.
> 
> I think there is a fundamental problem with the metadata design--it's
> ridiculous to assume that any pass that splits critical edges has
> LoopInfo--but that should at least workaround your problem.
> 
> Is that a flaw in the design of the metadata, or just in the fact
> that the utility functions used to manipulate the metadata are part
> of LoopInfo?
> 
> 
> I haven’t reviewed the metadata design since it was put in place.
> Based on this patch, the problem is that splitCriticalEdges needs to
> update the metadata (that is itself a somewhat bad design)

I believe this was a known issue when we discussed the design; if we can come up with a better design, that'd (by definition) be great. The core issue is just how to associate metadata with a loop.

>, and
> updating the metadata requires LoopInfo to determine whether we’re
> splitting the loop backedge or not. My suggestion is simply to use
> the DomTree to determine whether we’re splitting the backedge and
> update the metadata without requiring LoopInfo.

I agree that this makes sense.

 -Hal

> 
> 
> You’re correct that my approach cannot use the setLoopID() utility,
> but I think that’s only part of the problem.
> 
> 
> -Andy

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory