[PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction

Tue Jan 12 14:01:11 PST 2016

On Tue, Jan 12, 2016 at 1:36 PM Piotr Padlewski <piotr.padlewski at gmail.com>
wrote:

> @Daniel: I think Ray is right, because I remember that you had pretty the
> same concerns when I did the my patch for this:
> http://reviews.llvm.org/D12170. Unfortunatelly I think not all of your
> comments have made it to the review, but I am pretty sure that you were
> worried about the case with the switch, and the patch was right.
>
I believe it is right *for assume*, which was the conclusion on that thread.

But "dominatedbyEdge" is not a flag that tells us we are just caring about
assume :)

It makes it sound like it will work in other cases as well, and ... it
won't :)

Like I said, I need more time to read it again and try to recall it, but
> this patch seems promising.
>

I'm actually pretty convinced the patch will fix the issue. My concern at
this point is that this function is getting immensely confusing and
complicated to follow given the weird cases it now handles.
It also pretends to be more general than it is.

Certainly, i would not expect anyone who has not an amazingly detailed
understanding of GVN to be able to understand how to fix or debug this
function.

That is ... concerning.
At the point at which it is doing very different CFG tests for very
different cases, i'd rather see it split or something.

>
> Piotr
>
> 2016-01-12 22:21 GMT+01:00 Daniel Berlin <dberlin at dberlin.org>:
>
>>
>>
>> On Tue, Jan 12, 2016 at 1:02 PM, Yuanrui Zhang <yuanrui.zhang at intel.com>
>> wrote:
>>
>>> Ray added inline comments.
>>>
>>> ================
>>> Comment at: lib/Transforms/Scalar/GVN.cpp:2137
>>> @@ +2136,3 @@
>>> +/// If DominatesByEdge is false, it means that there might be multiple
>>> edges
>>> +/// from Root.Start to Root.End, in which case we need to pass
>>> Root.Start and
>>> +/// use a different implementation of replaceDominatedUsesWith to test.
>>> ----------------
>>> dberlin wrote:
>>> > If there are multiple edges to the same BB, we should not do the
>>> replacement at all (we lack post-dominance info necessary to be safe in
>>> that case).
>>> Note that, there is a condition about "multiple edges to the same BB":
>>> it is from Root.Start to Root.End. That's when DominatesByEdge is set to
>>> false, and that's exactly the reason why DominatesByEdge was introduced as
>>> a parameter to this function.  See the example added before
>>>
>>
>> This seems just wrong, for the reasons you've pointed out.
>>
>>>
>>>
>>> I was trying to make the comments in more detail. But if it is
>>> confusing, we can improve it.
>>>
>>> ================
>>> Comment at: lib/Transforms/Scalar/GVN.cpp:2201
>>> @@ -2197,3 +2200,3 @@
>>>                ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
>>> -              : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getEnd());
>>> +              : replaceDominatedUsesWith(LHS, RHS, *DT,
>>> Root.getStart());
>>>
>>> ----------------
>>> dberlin wrote:
>>> > If this problem only occurs with assume instructions, we shouldn't
>>> change the generic code.
>>> > If it's not a problem that only occur with assume instructions, we need
>>> > A. more testcases.
>>> > B. a more complete description of the problem that is happening and
>>> why this is the correct fix.
>>> >
>>> ===This is a general problem in GVN::propagateEquality with the
>>> following code:
>>>
>>> unsigned NumReplacements =
>>>           DominatesByEdge
>>>               ? replaceDominatedUsesWith(LHS, RHS, *DT, Root)
>>>               : replaceDominatedUsesWith(LHS, RHS, *DT, Root.getStart());
>>>
>>> When DominatesByEdge is TRUE, it passes the edge "Root" to
>>> replaceDominatedUsesWith, and we are fine. Since, if the edge "Root"
>>> dominates a USE, then Root.Start must dominates the USE.
>>>
>>
>> Yes, it must, but that does not make it safe :)
>>
>> If you propagate something through a switch statement, for example, it
>> will have different cases that go to the same block, but you can't replace
>> things in those blocks :)
>>
>> THat is, root.getStart will dominate all the uses, but you can't do *any*
>> replacement.
>> root.getEnd will not dominate all the uses, only the ones actually in the
>> target edge end.
>>
>>>
>>> When DominatesByEdge is FALSE, it passes the Root.End to
>>> replaceDominatedUsesWith, and we have a problem here.  Since, if Root.End
>>> dominates a USE, it does not mean that Root.Start dominates the USE. We
>>> show in the test case _Z1im below.
>>>
>>
>>> bool RootDominatesEnd = DT.dominates(Root.Start(), Root.End()), then it
>>> can be used:
>>>
>>
>> This will hit the multiple edge case, sadly.
>>
>>
>>> In fact, the simplest way is to
>>> 1) pass Root.Start  to replaceDominatedUsesWith,
>>> 2) then change replaceDominatedUsesWith(... BasicBlock* BB)  to replace
>>> the uses by the "END" of the BB.
>>> These two changes together will give exactly the same effect as when
>>> passing an edge, i.e. ,replaceDominatedUsesWith(.....   BasicBlockEdge
>>> Root).
>>>
>>> I'm not sure this is worth the confusion/cost.
>>
>> This seems *super* complicated.
>>
>>
>>> Note that, currently GVN is the only caller of replaceDominatedUsesWith(
>>> ...,    BasicBlock* BB).  So, it is safe to change to use
>>> "properlyDominates".
>>>
>>>
>> It's safe, but confusing.
>>
>>>
>>> ================
>>> Comment at: lib/Transforms/Utils/Local.cpp:1540
>>> @@ -1539,3 +1539,3 @@
>>>      auto *I = cast<Instruction>(U.getUser());
>>> -    if (DT.dominates(BB, I->getParent())) {
>>> +    if (DT.properlyDominates(BB, I->getParent())) {
>>>        U.set(To);
>>> ----------------
>>> dberlin wrote:
>>> > This is definitely not correct.
>>> > Local to a block, all uses are dominated by their definition, so you
>>> don't need properlyDominates to replace them legally.
>>> > It's up to the caller to determine whether this is safe *semantically*.
>>> >
>>> > (Even if this change was correct, you've also now made the replacement
>>> functions completely inconsistent with each other.)
>>> Since propagateEquality in GVN is the only caller, that's why we changed
>>> the comments to inform other users that, this function will replace USES
>>> from the END of the BB.
>>>
>>> If the called wants to replace the uses in the BB itself, the other
>>> overloaded interface replaceDomiantedUsesWith(..... BasicBlockEdge Root)
>>> can be used.
>>>
>>
>>
>> I'd like to hear Piotr's thoughts on all of this.
>>
>> My view is that if we have to make propagateEquality this complicated to
>> support assume, we should seriously reconsider how we do propagateEquality
>> (or split propagateEquality into propagateEquality and propagateAssume, or
>> *something*).
>>
>> What is being done here is just making an already hard-to-understand
>> function *much* harder to understand.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160112/9756d6c4/attachment-0001.html>