[PATCH][instcombine] Remove preservesCFG from instcombine

[Nick Lewycky]

Hal Finkel wrote:
> ----- Original Message -----
>> From: "Nick Lewycky"<nicholas at mxc.ca>
>> To: "Mark Heffernan"<meheff at google.com>
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Tuesday, November 4, 2014 2:28:20 AM
>> Subject: Re: [PATCH][instcombine] Remove preservesCFG from instcombine
>>
>> Mark Heffernan wrote:
>>> Hi Andrew,
>>>
>>> (Let me know if someone else is more appropriate to review this).
>>>   This
>>> patch removes setPreservesCFG() from the instcombine pass.
>>
>> NAK. Please don't commit this.
>
> Already happened (r221223) [but, via the wonders of version control...]

Fair enough. I would like it changed, either fixed forwards or backed 
out, but this is not urgent and we can decide what the right thing to do 
is first.

>>     The reason
>>> is that InstCombine can modify terminator instructions (branches)
>>> specifically the predicate.
>>
>> This is the thing where it removes an '%newcond = xor %cond, true'
>> and
>> switches 'br %newcond, label %A, label %B' with 'br %cond, label %B,
>> label %A'?
>>
>> If it's actually causing you problems, please just move that
>> transform
>> to simplifycfg instead of removing preserves-cfg from instcombine.
>
> It seems like the problem is more general than that:
>
>>> The reason is that InstCombine can modify terminator instructions (branches) specifically the predicate.  Instcombine clears out dead blocks and replaces all the dead instruction uses with undef:
>>>
>>> http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792
>>>
>>> These new undefs can be predicates of branches, and from the comment of setPreservesCFG():
>
> So perhaps any optimization that produces an undef, where that undef could possibly propagate to a branch condition, could under iteration cause this problem. And the code above there says:
>
>      // Do a quick scan over the function.  If we find any blocks that are
>      // unreachable, remove any instructions inside of them.  This prevents
>      // the instcombine code from having to deal with some bad special cases.
>
> So this may actually be trickier to solve otherwise than it at-first appears.

Clearly a CFG preserving pass may change the predicate to a branch. An 
optimization which determines that i1 %x is always true may run 
%x->RAUW(i1 true), it would be terrible if preserving the CFG would 
prevent that.

This implies that CFG driven analyses need to be prepared to have the 
branch predicates (and switch predicates, and invoke arguments) change 
under them, so long as the change doesn't have a semantic difference. If 
an SSA value gets replaced with undef, that's because it always was 
undef, it just wasn't easy to see previously. So I think the place to 
look is at LoopInfo and its handling of undef. Currently we treat undef 
as "can branch to either side, not sure which". What is LoopInfo doing?

Nick

>
>   -Hal
>
>>
>> Nick
>>
>>     Instcombine clears out dead blocks and
>>> replaces all the dead instruction uses with undef:
>>>
>>> http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l02792
>>>
>>> These new undefs can be predicates of branches, and from the
>>> comment
>>> of setPreservesCFG():
>>>
>>> // setPreservesCFG - This function should be called to by the pass,
>>> iff
>>> they do
>>> // not:
>>> //
>>> //  1. Add or remove basic blocks from the function
>>> //  2. Modify terminator instructions in any way.
>>>
>>> Clearly instcombine is modifying terminator instructions so should
>>> not
>>> be calling setPreservesCFG().  The specific issue I ran into was a
>>> dead
>>> loop not being deleted because of stale analysis data.  Repro:
>>>
>>> void foo(int *a, int start_x, int end_x, int start_y, int end_y,
>>> int k) {
>>>     for (int y = start_y; y<  end_y; y++) {
>>>       for (int x = start_x; x<  end_x; x++) {
>>>         for (int i = 0; i<  1000; i++) {
>>>           a[x + y + k*i] = 0;
>>>         }
>>>       }
>>>     }
>>> }
>>>
>>> # Dead loop left after loop deletion:
>>> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate
>>> -loop-unswitch
>>> -instcombine -loop-deletion -S>  bad.ll
>>>
>>> # Dead loop removed by splitting out loop-deletion into separate
>>> opt
>>> invocation:
>>> clang -S -emit-llvm foo.c -o - | opt -O1 -loop-rotate
>>> -loop-unswitch
>>> -instcombine | opt -loop-deletion -S>  good.ll
>>>
>>> This patch fixes this discrepancy.
>>>
>>> Mark
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>