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

[Daniel Berlin via llvm-commits]

On Mon, Jul 17, 2017 at 2:04 AM, Pedro Ferreira via Phabricator <
reviews at reviews.llvm.org> wrote:

> PFerreira created this revision.
> Herald added a reviewer: dberlin.
>
> Related to bugzilla 33731 https://bugs.llvm.org/show_bug.cgi?id=33731
>
> This is caused by a a PRE insertion when the predecessor block hasn't yet
> been through Value Numbering (due to a back-edge).
>

I'm not surprised this is buggy, sadly.
I suspect there are a bunch of issues around this kind of thing.

> The PRE insertion adds a duplicate instruction at the end of the block,
> but the already-existing instruction is already used in the predecessor
> block, like so:
>
> L.LB1_346:                                        ; preds = %L.LB1_423
>
>   %7 = sext i32 %2 to i64
>   %8 = mul i64 %7, 4
>
>

> - %.pre = sext i32 %1 to i64** ; inserted by GVN::performScalarPREInsertion
>
> When we then iterate over this block (later in the pass), GVN chooses to
> delete the first instance of the instruction as it is duplicated (instead
> of the later one which was created before).

This sounds like an issue with findLeader/addToLeaderTable not checking
local dominance if they are in the same block, based on the assumption that
it will process the block in order.
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.

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.

> I though about stoping "GVN::performScalarPREInsertion" from inserting
> the instruction by numbering the BasicBlock, but that might lead to
> infinite loops. I also tried to stop PREInsertion from running if the
> BasicBlock had not been numbered yet (by keeping a list of numbered blocks)
> but that wouldn't work since GVN only does one pass over the function (so
> this case would never be optimised out).
>
> The patch does fix the bug though, without adding regressions.
>
>
> https://reviews.llvm.org/D35475
>
> Files:
>   lib/Transforms/Scalar/GVN.cpp
>
>
> Index: lib/Transforms/Scalar/GVN.cpp
> ===================================================================
> --- lib/Transforms/Scalar/GVN.cpp
> +++ lib/Transforms/Scalar/GVN.cpp
> @@ -1807,6 +1807,13 @@
>      return false;
>    }
>
> +  // If this was a shortcut PRE, it will have been inserted at the end
> +  // of the block; ensure that it dominates all uses of the original.
> +  if (Instruction *R = dyn_cast<Instruction>(Repl)) {
> +    if (DT->dominates(I, R)) {
> +      R->moveBefore(I);
> +    }
> +  }
>    // Remove it!
>    patchAndReplaceAllUsesWith(I, Repl);
>    if (MD && Repl->getType()->isPtrOrPtrVectorTy())
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170717/a6f6f43b/attachment.html>