-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Miscompile in DAGCombine #80911
Comments
Are you sure your code is correct? There are some issues with alignments, because of that this code returns different results. Check how you transform your test |
The LLVM IR was generated from Rust's MIR code. The only changes I made was substituting out It's also possible that the bug is in Rust's codegen. Can you point out where the alignment issue is? |
Here is the IR I got after transformation:
See the comment |
I think this is a bug in DAGCombine, specifically with this code in MatchLoadCombine: https://github.com/llvm/llvm-project/blame/7ddc32052546abd41656d2e670f3902b1bf805a7/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L9248-L9255 That "transfer chain users" logic does not look correct to me for the cases where the original loads do not get removed. In that case, their chains will no longer be used at all, which will permit those loads to be incorrectly reordered. |
The |
I believe this is a fairly minimal reproducer: define i32 @test(ptr %ptr, ptr %clobber) {
%load = load <4 x i8>, ptr %ptr, align 16
store i32 0, ptr %clobber
store <4 x i8> %load, ptr %ptr, align 16
%e1 = extractelement <4 x i8> %load, i64 1
%e1.ext = zext i8 %e1 to i32
%e1.ext.shift = shl nuw nsw i32 %e1.ext, 8
%e0 = extractelement <4 x i8> %load, i64 0
%e0.ext = zext i8 %e0 to i32
%res = or i32 %e1.ext.shift, %e0.ext
ret i32 %res
} Produces:
|
The load combine replaces a number of original loads with one new loads and also replaces the output chains of the original loads with the output chain of the new load. This is only correct if the old loads actually get removed, otherwise they may get incorrectly reordered. The code did enforce that all involved operations are one-use (which also guarantees that the loads will be removed), with one exceptions: For vector loads, multi-use was allowed to support multiple extract elements from one load. This patch collects these extract elements, and then validates that the loads are only used inside them. I think an alternative fix would be to replace the uses of the old output chains with TokenFactors that include both the old output chains and the new output chain. However, I think the proposed patch is preferable, as the profitability of the transform in the general multi-use case is unclear, as it may increase the overall number of loads. Fixes llvm#80911.
The load combine replaces a number of original loads with one new loads and also replaces the output chains of the original loads with the output chain of the new load. This is incorrect if the original load is retained (due to multi-use), as it may get incorrectly reordered. Fix this by using makeEquivalentMemoryOrdering() instead, which will create a TokenFactor with both chains. Fixes #80911.
/cherry-pick 25b9ed6 |
Failed to cherry-pick: 25b9ed6 https://github.com/llvm/llvm-project/actions/runs/7889150888 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
The load combine replaces a number of original loads with one new loads and also replaces the output chains of the original loads with the output chain of the new load. This is incorrect if the original load is retained (due to multi-use), as it may get incorrectly reordered. Fix this by using makeEquivalentMemoryOrdering() instead, which will create a TokenFactor with both chains. Fixes llvm#80911. (cherry picked from commit 25b9ed6)
Backport PR opened at #81633. |
Reproduction:
Right:
Wrong:
Bisected down to
On trunk 5a9af39
From fuzzer-generated Rust. IR emitted with
-Zmir-opt-level=0 -Copt-level=0
and then Rust std symbols manually shimmed.Rust MIR
The text was updated successfully, but these errors were encountered: