[PATCH][instcombine] Remove preservesCFG from instcombine

[Hal Finkel]

----- 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...]

> 
>    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.

 -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
> 

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