[PATCH] D35475: [PATCH] [GVN] Ensure replacement instruction dominates its uses

[Daniel Berlin via llvm-commits]

I replied on the review, if you stare at the last paragraph, i'm very
confused how this bug could ever occur at all.


>
> addToLeaderTable always adds to the end, and so it will hit the PRE leader
> before any other.
>
> If so, the fastest fix i can think of is to  number the instructions in
> the block starting at 0 as we process them. An instruction can only be a
> leader when BB = LeaderBB iff the number is < than the current
> instruction's number.
> Assign all PRE'd instructions numbers that are MAX_INT.
>
>
> I'm not sure I understand - the instruction in particular has already been
> numbered on a different block.
>
By numbering above, i mean literally assign them numbers starting at 0. IE
DFS number them.
Not value number them.
See what NewGVN does with local numbers when computing the elimination
order for instructions,.

The dominance ordering of instructions is completely described by <order of
blocks in dominator tree, order of instructions in block>.  Numbering them
as you see them provides <order in block>.



>
> If you have two MAX_INT instructions, they are in-order already (since PRE
> will only add them in order).
>
>
>
>
>> This is because "GVN::performScalarPREInsertion" will number the
>> instruction as it is created (so it is in the leader-list).
>
>
>> The attached patch moves the "%.pre" instruction up so that the
>> replacement is valid.
>>
>> I am not sure this is a valid fix, but I couldn't figure out another
>> solution.
>>
> There are a couple other ways to fix this.
> Another would be "always check local dominance using OrderedInstruction in
> FindLeader ". This is equivalent to the above.
>
>
> But wouldn't that require that the instruction exists in the leader list?
>
No.
You'd do findLeader(CurInst), and compare local dominance of the leader and
curinst.  Leader can't be a leader if the number you gave it is > number
for curinst.


> If I do check the dominance, wouldn't I have to scan the incoming block
> anyway?
>
> No, that's why you number them above.


> I do have another alternative; just before where I added the instruction
> moving, we have:
>
>   Value *Repl = findLeader(I->getParent(), Num);
>   if (!Repl) {
>     // Failure, just remember this instance for future use.
>     addToLeaderTable(Num, I, I->getParent());
>     return false;
>   } else if (Repl == I) {
>     // If I was the result of a shortcut PRE, it might already be in the
> table
>     // and the best replacement for itself. Nothing to do.
>     return false;
>   }
>
> we could append to this if-else block:
>
> else if (DT->dominates(I, R)) {
>   // Replace the existing leader since this one dominates it.
>   replaceOnLeaderTable(Num, Repl, I);
> }
>
> This way we'd preserve the existing instruction (instead of replacing it
> with the "%.pre" one).
>

This seems like a hack :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170717/fd992497/attachment.html>