[llvm-commits] [PATCH] Fix llvm.invariant support

[Nick Lewycky]

Kenneth Uildriks wrote:
> On Sun, Dec 5, 2010 at 4:49 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>> Kenneth Uildriks wrote:
>>>
>>> Here's the updated version.
>>
>> Firstly, could you please reduce your tests? Try writing tests as .ll files
>> by hand; they should be absolutely minimal to expose the optimization (or
>> verify that we don't do an unsafe transformation).
>>
>> +    DEBUG(errs()<<  "Invariant value is ("<<  *(BI->first)<<  ","
>> +<<  BI->second.Size<<  ")\n");
>> +    DEBUG(errs()<<  "Target is ("<<  *MemLoc.Ptr<<  ", "<<  MemLoc.Size
>> +<<  ")\n");
>>
>> These should be DEBUG(dbgs()<<  ...) not errs(). Please check all of them.
>>
>> Nick
>>
> After reducing the tests, I found cases that it missed, so I reworked
> the algorithm.  Now it recognizes invariant regions as regions of code
> dominated by an llvm.invariant.begin that aren't reachable from the
> matching llvm.invariant.end without going back through the
> llvm.invariant.begin.  It brings in the DominatorTree and
> DominanceFrontier analyses to figure this out more quickly and
> cleanly.
>
> In MemDepPrereqs.patch, there are changes to DominanceFrontier and
> BreakCritical edges that were needed for my MemoryDependenceAnalysis
> changes to pass all the nightly tests.  Since DominanceFrontier is
> really a cache of frontier analysis results, it seems to me that it
> shouldn't be asserting that an add attempt isn't redundant (and a
> later pass was trying to add something that my algorithm had already
> looked up in a few cases).  Also, there was a latent bug in
> BreakCriticalEdges that allows an llvm_unreachable assertion to
> occasionally hit if DominanceFrontier is in play and a block is
> unreachable.

Let's do this one piece at a time. What's the bug in critical edges 
splitting? Can you produce a .ll file that fails "opt -break-crit-edges"?

Broadly, I'm not happy about the domtree changes. I don't see why 
clients should need to query about blocks that don't exist, or add 
blocks that are already added. This indicates that extra work is going 
on at the callers side at the very least, and real problems some of the 
time.

Adding DomTree as a dependency of MemDep is something that should be 
agreed upon with its maintainers first, but I'm for it. I've wanted to 
add other enhancements to memdep before and stopped due to a lack of 
domtree, and we know that domtree is preserved by almost every pass in 
the tree.

Nick