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

[CallGraph] New llvm.assume might not show up in GlobalsAA/CallGraph #63925

Closed
jdoerfert opened this issue Jul 17, 2023 · 3 comments
Closed

[CallGraph] New llvm.assume might not show up in GlobalsAA/CallGraph #63925

jdoerfert opened this issue Jul 17, 2023 · 3 comments
Labels
invalid Resolved as invalid, i.e. not a bug llvm:optimizations

Comments

@jdoerfert
Copy link
Member

jdoerfert commented Jul 17, 2023

After we create the CallGraph, we might add a llvm.assume call. They need to be added to the CallGraph, or GlobalsAA thinks the function is side-effect free.

I don't have a small reproducer. I found this because @_X_strcmp in llvm-test-suite/MultiSource/Applications/spiff/compare.c is marked memory(read) even though it contains a (non-trivial) llvm.assume call.

@jdoerfert jdoerfert changed the title [Call] New llvm.assume might not show up in the CallGraph [CallGraph] New llvm.assume might not show up in the CallGraph Jul 17, 2023
@jdoerfert
Copy link
Member Author

jdoerfert commented Jul 17, 2023

Test case: https://godbolt.org/z/1jbvKPzEn

define internal void @foo(ptr %p) {
  %c = icmp eq ptr %p, null
  br i1 %c, label %err, label %ok
err:
  unreachable
ok:
  ret void
}
define void @bar() {
  call void @foo()
  ret void
}

Run with
-p="require<globals-aa>,simplifycfg,function-attrs"

Results in memory(none) even though there is an llvm.assume (=not good).

@jdoerfert jdoerfert changed the title [CallGraph] New llvm.assume might not show up in the CallGraph [CallGraph] New llvm.assume might not show up in GlobalsAA/CallGraph Jul 17, 2023
jdoerfert added a commit to jdoerfert/llvm-project that referenced this issue Jul 18, 2023
When SimplifyCFG, and probably other passes, create a call to
`llvm.assume` to retain some knowledge, they need to update the state of
GlobalsAA. If not, later passes might think the function is side-effect
free, or read only, which it is not, due to the `llvm.assume` call.
Deducing the wrong memory effects can lead to problems, e.g., hoisting
of a call and thereby introducing UB.

Fixes: llvm#63925
@jdoerfert
Copy link
Member Author

Tag (@nikic @fhahn @aeubanks)

@jdoerfert
Copy link
Member Author

Invalid, see discussion here: https://reviews.llvm.org/D155528#4509210

@jdoerfert jdoerfert closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2023
@EugeneZelenko EugeneZelenko added the invalid Resolved as invalid, i.e. not a bug label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Resolved as invalid, i.e. not a bug llvm:optimizations
Projects
None yet
Development

No branches or pull requests

2 participants