Jump Theading/GVN bug

[Rafael Espíndola]

I agree. I think we cannot simply change the verifier since some
passes (instsimplify) do have to handle unreachable code and we have
to test them. We could have a -verify-no-dead-bb mode and could even
make it the default.

On 23 February 2015 at 11:21, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>
> On Mon Feb 23 2015 at 7:38:37 AM Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>
>> >> This instruction doesn’t appear to be well formed. However, one could
>> >> argue that it is – formally %2 def does dominate all %2 uses because
>> >> both
>> >> are located in an unreachable block, so every path to the use is
>> >> passing
>> >> through the def (because there are 0 paths like that).
>> >>
>> >> That is likely the reason why –verify doesn’t complain about it.
>> >
>> >
>> > Why is jump-threading creating this funky instruction?
>>
>> It is well formed. I agree with Hal: it might be a quality improvement
>> to generate tidier IR from jump-threading, but GVN has to handle
>> whatever input passes the verifier.
>
>
> Then, IMHO, we should start the long process of making forward-unreachable
> code not acceptable except inside a single pass (IE at the end of each pass,
> there should be no unreachable code).
>
> There are a very very small number of passes that generate such code, and
> it's not that difficult to fix them to make them not do that.
>
> On the other hand, having *all* passes required to handle pretty much
> garbage, as long as it's unreachable, has a pretty high cost.
>
> GVN already spends a bunch of code and time trying to deal with the
> weirdness of unreachable code, as do a lot of other passes (For reference,
> 5-10% of GVN's testcases deal with making sure it doesn't crash on
> unreachable code, depending on how you count)
>
> At least a few other passes i've dealt with in the past week also go out of
> their way, after dealing with normal code, to deal  with unreachable code
> just to avoid crashing themselves later. So we are wasting compile time and
> maintenance burden on it too.
>
> Worse, they almost all have bugs in how they handle it (I found two this
> week while using a utility class for something).
> Fixing them all to handle unreachable code "properly" (since there is no
> real proper thing to do in most cases except treat it as radioactive parts
> of the input that you shouldn't really touch, but still have to do something
> with,  since phi nodes may have unreachable predecessors, etc), IMHO, brings
> no appreciable benefit to the compiler, particularly when the alternative is
> "make it illegal".
>
>>
>> The GVN crashes reduces to just running GVN in
>>
>> define i8 @foo(i8* %x) {
>> bb0:
>>   br label %bb2
>> bb1:
>>   %tmp1 = getelementptr inbounds i8* %tmp1, i64 1
>>   br i1 false, label %bb2, label %bb1
>> bb2:
>>   %tmp2 = phi i8* [ %tmp1, %bb1 ], [ %x, %bb0 ]
>>   %tmp3 = load i8* %tmp2, align 1
>>   ret i8 %tmp3
>> }
>>
>> Cheers,
>> Rafael
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits