[llvm-commits] [Review] GVN Patch

[Bill Wendling]

On Oct 19, 2012, at 2:40 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Fri, Oct 19, 2012 at 2:20 PM, Bill Wendling <wendling at apple.com> wrote:
>> For those of you wondering why I was so interested in dominance, there is a situation where GVN is barfing because it goes into an infinite loop. The problem is that it's trying to process a non-local load which has a dependency in a block that's unreachable from the entry point. It gets into an infinite loop because the GEP it is querying is of this form:
>> 
>>        %xx = getelementptr %some.type %xx ...
>> 
>> which I'm told is perfectly valid within unreachable blocks.
>> 
>> Below (inlined because it's so small) is my fix for this. It at least allows the code to compile to completion. I haven't worked with GVN much before, so I don't know if this has any non-local effects.
>> 
>> Please review this and let me know if it's okay to commit. (And I will commit a testcase with this once it's been approved.)
>> 
>> Thanks.
>> 
>> -bw
>> 
>> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp
>> index e67d8af..5411786 100644
>> --- a/lib/Transforms/Scalar/GVN.cpp
>> +++ b/lib/Transforms/Scalar/GVN.cpp
>> @@ -1368,6 +1368,8 @@ bool GVN::processNonLocalLoad(LoadInst *LI) {
>>     BasicBlock *DepBB = Deps[i].getBB();
>>     MemDepResult DepInfo = Deps[i].getResult();
>> 
>> +    if (!DT->isReachableFromEntry(DepBB)) continue;
>> +
>>     if (!DepInfo.isDef() && !DepInfo.isClobber()) {
>>       UnavailableBlocks.push_back(DepBB);
>>       continue;
> 
> This isn't wrong, exactly... but the check is later than it should be:
> memdep shouldn't be returning results for unreachable blocks in the
> first place.
> 
Owen expressed doubt about that when I asked him because memdep uses caching and stuff. Owen, could you shed some light on what your concerns were?

-bw