Skip to content
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

[LoopVectorize] Miscompile with iteration-local scoped alias metadata #79137

Closed
nikic opened this issue Jan 23, 2024 · 5 comments · Fixed by #79161
Closed

[LoopVectorize] Miscompile with iteration-local scoped alias metadata #79137

nikic opened this issue Jan 23, 2024 · 5 comments · Fixed by #79161

Comments

@nikic
Copy link
Contributor

nikic commented Jan 23, 2024

C reproducer: https://clang.godbolt.org/z/f6rE4jvhM

IR reproducer: https://llvm.godbolt.org/z/7anbPon8K

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define hidden void @test(ptr %arg, i64 %num) {
entry:
  %icmp = icmp ult i64 %num, 2
  br i1 %icmp, label %exit, label %preheader

preheader:
  %arg.1 = getelementptr inbounds i8, ptr %arg, i64 1
  %end = add i64 %num, -2
  br label %loop

loop:
  %prev.ptr = phi ptr [ %cur.ptr, %loop ], [ %arg, %preheader ]
  %iv = phi i64 [ %iv.next, %loop ], [ 0, %preheader ]
  %cur.ptr = getelementptr inbounds i8, ptr %arg.1, i64 %iv
  call void @llvm.experimental.noalias.scope.decl(metadata !0)
  call void @llvm.experimental.noalias.scope.decl(metadata !3)
  %load.prev = load i8, ptr %prev.ptr, align 1, !alias.scope !0, !noalias !3
  %load.cur = load i8, ptr %cur.ptr, align 1, !alias.scope !3
  %add = add i8 %load.cur, %load.prev
  store i8 %add, ptr %cur.ptr, align 1, !alias.scope !3
  %iv.next = add nuw i64 %iv, 1
  %cmp = icmp eq i64 %iv, %end
  br i1 %cmp, label %exit, label %loop

exit:
  ret void
}

; Function Attrs: nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: readwrite)
declare void @llvm.experimental.noalias.scope.decl(metadata) #0

attributes #0 = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: readwrite) }

!0 = !{!1}
!1 = distinct !{!1, !2}
!2 = distinct !{!2}
!3 = !{!4}
!4 = distinct !{!4, !5}
!5 = distinct !{!5}

The current and previous value are only noalias within the iteration (per the location of the @llvm.experimental.noalias.scope.decl), but the vectorization appears to assume that the underlying objects don't alias.

@nikic
Copy link
Contributor Author

nikic commented Jan 23, 2024

Well, I guess the actual problem here is in LAA rather than LoopVectorize proper. Debug log:

LAA: Found a loop in test: loop
LAA: Processing memory accesses...
  AST: Alias Set Tracker: 2 alias sets for 2 pointer values.
  AliasSet[0x8e36e00, 1] must alias, No access Memory locations: (ptr %cur.ptr, unknown before-or-after)
  AliasSet[0x8e372b0, 1] must alias, No access Memory locations: (ptr %prev.ptr, unknown before-or-after)

LAA:   Accesses(3):
	  %cur.ptr = getelementptr inbounds i8, ptr %arg.1, i64 %iv (write)
	  %prev.ptr = phi ptr [ %cur.ptr, %loop ], [ %arg, %preheader ] (read-only)
	  %cur.ptr = getelementptr inbounds i8, ptr %arg.1, i64 %iv (read)
Underlying objects for pointer   %cur.ptr = getelementptr inbounds i8, ptr %arg.1, i64 %iv
  ptr %arg
Underlying objects for pointer   %cur.ptr = getelementptr inbounds i8, ptr %arg.1, i64 %iv
  ptr %arg
Underlying objects for pointer   %prev.ptr = phi ptr [ %cur.ptr, %loop ], [ %arg, %preheader ]
  ptr %arg
LAA: May be able to perform a memory runtime check if needed.
LAA: No unsafe dependent memory operations in loop.  We don't need runtime memory checks.

When constructing the AST we get the LocationSize to BeforeOrAfter, but we retain the AATags, which is generally not valid.

@fhahn

@nikic nikic self-assigned this Jan 23, 2024
nikic added a commit that referenced this issue Jan 23, 2024
nikic added a commit to nikic/llvm-project that referenced this issue Jan 23, 2024
LAA currently adds memory locations with their original AATags to
AST. However, scoped alias AATags may be valid only within one
loop iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the
loop, and drop AATags that reference these scopes.

Fixes llvm#79137.
nikic added a commit to nikic/llvm-project that referenced this issue Jan 24, 2024
LAA currently adds memory locations with their original AATags to
AST. However, scoped alias AATags may be valid only within one
loop iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the
loop, and drop AATags that reference these scopes.

Fixes llvm#79137.
@nikic nikic reopened this Jan 26, 2024
@nikic nikic added this to the LLVM 18.X Release milestone Jan 26, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jan 26, 2024
@nikic
Copy link
Contributor Author

nikic commented Jan 26, 2024

/cherry-pick 543cf08 cd7ea4e

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Jan 26, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
Copy link

/pull-request #79561

@nikic nikic moved this from Needs Triage to Needs Review in LLVM Release Status Jan 26, 2024
@mjklemm mjklemm closed this as completed Jan 26, 2024
@EugeneZelenko
Copy link
Contributor

@mjbedy: But pull request is still open.

@nikic nikic reopened this Jan 27, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Jan 28, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
nikic added a commit to nikic/llvm-project that referenced this issue Feb 1, 2024
(cherry picked from commit 0c02b2e)
nikic added a commit to nikic/llvm-project that referenced this issue Feb 1, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
@tstellar
Copy link
Collaborator

tstellar commented Feb 2, 2024

PR has been created, we will track the status there.

@tstellar tstellar closed this as completed Feb 2, 2024
@tstellar tstellar moved this from Needs Review to Done in LLVM Release Status Feb 2, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 3, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
@tstellar tstellar moved this from Done to Needs Pull Request in LLVM Release Status Feb 3, 2024
@tstellar tstellar reopened this Feb 3, 2024
@tstellar tstellar closed this as completed Feb 3, 2024
@tstellar tstellar moved this from Needs Pull Request to Done in LLVM Release Status Feb 3, 2024
cuviper pushed a commit to rust-lang/llvm-project that referenced this issue Feb 13, 2024
(cherry picked from commit 0c02b2e)
cuviper pushed a commit to rust-lang/llvm-project that referenced this issue Feb 13, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
MingcongBai pushed a commit to AOSC-Tracking/llvm-project that referenced this issue Mar 26, 2024
(cherry picked from commit 0c02b2e)
MingcongBai pushed a commit to AOSC-Tracking/llvm-project that referenced this issue Mar 26, 2024
…lvm#79161)

LAA currently adds memory locations with their original AATags to AST.
However, scoped alias AATags may be valid only within one loop
iteration, while LAA reasons across iterations.

Fix this by determining which alias scopes are defined inside the loop,
and drop AATags that reference these scopes.

Fixes llvm#79137.

(cherry picked from commit cd7ea4e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment