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

Daniel Berlin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 17 09:10:15 PDT 2017


dberlin added a comment.

In https://reviews.llvm.org/D35475#811435, @PFerreira wrote:

> I set a breakpoint in the R->moveBefore(I) which I added and this was the block in question:
>
>   L.LB1_346:                                        ; preds = %L.LB1_423
>     %6 = sext i32 %1 to i64
>     %7 = mul i64 %6, 4
>     %8 = getelementptr i8, i8* null, i64 %7
>     %9 = bitcast void (...)* @poisn2_ to void ([...])*
>     call [...]
>     %.pre = sext i32 %1 to i64
>     br label %L.LB1_350
>
>
> The way I debugged this was to track the creation of the "%.pre" instruction. This led me to "GVN::performScalarPREInsertion". I set a breakpoint in that function and it gets hit before the BasicBlock above has a chance to go through "processBlock" (during iterateOnFunction). Hence the %7 has not been found yet (not yet in the leader list). This violates the assumption in performScalarPREInsertion, as you stated before (I think) which is documented by the comment in that function:


This is, in fact, because of the GEP case, where it is PRE'ing in the middle of value numbering.
There is no other case it does this.

This seems fairly dangerous.
I'd actually really like to understand if this produces any real performance improvements, because i'd rather just disable this.
There are a bunch of cases we know such things will fail (for example, if GVN has discovered dead code, etc).

Because GVN's VN is so weak, we may be able to prove it safe, but it seems ... a bad plan.


https://reviews.llvm.org/D35475





More information about the llvm-commits mailing list