[PATCH][instcombine] Remove preservesCFG from instcombine

[Mark Heffernan]

It seems to me that LoopSimplify is the problem here.  Should
LoopSimplifyPass be marked as IsCFGOnlyPass?  It clearly modifies the CFG.
>From the "writing an llvm pass" manual:  "if a pass walks CFG without
modifying it then the third argument [isCFGOnly] is set to true", however,
I don't know how definitive that is and the logic of that statement doesn't
exactly imply that a CFG-modifying pass can't be marked isCFGOnly.  If
LoopSimplifyPass is changed to !isCFGOnly, then preservesCFG can be added
back to instcombine (at least for the purposes of the bug I ran into).

Mark

On Tue, Nov 4, 2014 at 11:23 AM, Nick Lewycky <nicholas at mxc.ca> wrote:

> 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
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141104/9d2050ae/attachment.html>