[PATCH] D34135: [JumpThreading] Use DT to avoid processing dead blocks

[Brian M. Rzycki via llvm-commits]

Thanks for the explanation Daniel. If it's the case we can occasionally
expect invalid IR then I'd like to move forward with reviewing this patch
to fix D34135. What is the process for me to follow?

On Tue, Mar 6, 2018 at 4:38 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> On Tue, Mar 6, 2018 at 2:18 PM, Brian Rzycki via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> brzycki added a comment.
>>
>> I have analyzed this failure and have what I believe (in @dberlin 's
>> words) to be the smallest hammer fix:
>>
>>   $ git diff lib/Analysis/LazyValueInfo.cpp
>>   diff --git a/llvm/lib/Analysis/LazyValueInfo.cpp
>> b/llvm/lib/Analysis/LazyValueInfo.cpp
>>   index 65fd007dc0b..36e66f7f6ef 100644
>>   --- a/llvm/lib/Analysis/LazyValueInfo.cpp
>>   +++ b/llvm/lib/Analysis/LazyValueInfo.cpp
>>   @@ -1145,9 +1145,17 @@ getValueFromConditionImpl(Value *Val, Value
>> *Cond, bool isTrueDest,
>>                 (!isTrueDest && BO->getOpcode() != BinaryOperator::Or))
>>        return ValueLatticeElement::getOverdefined();
>>
>>   -  auto RHS = getValueFromCondition(Val, BO->getOperand(0), isTrueDest,
>> Visited);
>>   -  auto LHS = getValueFromCondition(Val, BO->getOperand(1), isTrueDest,
>> Visited);
>>   -  return intersect(RHS, LHS);
>>   +  // Prevent infinite recursion if Cond references itself as in this
>> example:
>>   +  //  Cond: "%tmp4 = and i1 %tmp4, undef"
>>   +  //    BL: "%tmp4 = and i1 %tmp4, undef"
>>   +  //    BR: "i1 undef"
>>   +  Value *BL = BO->getOperand(0);
>>   +  Value *BR = BO->getOperand(1);
>>   +  if (BL == Cond || BR == Cond)
>>   +    return ValueLatticeElement::getOverdefined();
>>   +
>>   +  return intersect(getValueFromCondition(Val, BL, isTrueDest,
>> Visited),
>>   +                   getValueFromCondition(Val, BR, isTrueDest,
>> Visited));
>>    }
>>
>> The infinite recursion happens because `Visited` is not altered or
>> updated until `getValueFromConditionImpl()` returns a value. In this case
>> `getValueFromConditionImpl()` calls `getValueFromCondition()` which calls
>> back to `getValueFromConditionImpl()` in an attempt to resolve the LHS and
>> RHS of a valid `BinaryOperator`.
>>
>> In our case this specific instruction we have `%tmp4 = and i1 %tmp4,
>> undef` which I don't quite understand. Shouldn't this be invalid SSA form?
>>
>
>
> We allow somewhat illegal IR in unreachable blocks.
> There are some mailing list discussions around this.
>
> This is under the guise of it  being easier than knowing when the block is
> dead and removing it.
> In my experience, it generates more bugs trying to avoid illegal IR than
> it is worth, and most passes end up trying to find a way to at least avoid
> visiting dead blocks.
> (Some try to be resilient and visit dead blocks, but they rarely survive
> it over time, and you will see a whole host of bugzilla bugs in this
> category :P)
>
> Handling this is much easier in non-lazy passes.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180307/92952c1e/attachment.html>