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

Attempt to fix infinite loop miscompilation from LLVM side #73561

Closed
wants to merge 1 commit into from

Conversation

sfanxiang
Copy link
Contributor

This works by marking all function calls as mayHaveSideEffects in LLVM, preventing it from deleting a call to non-halting function. Although it looks simple, this does fix all repros I could run from #28728.

It seems LLVM does not delete a loop just because it's not making progress (see this comment). However, LLVM can delete a call to a function containing that loop. This patch prevents the latter.

This has several advantages over #59546:

  • All codegen tests pass (in local test).
  • It's harder to cause a regression, because it's rare to call a function and use neither its return value or side effect.
  • No extra codegen. @llvm.sideeffect seems difficult to use efficiently. I observed significant slowdown even when combining @llvm.sideeffect after every function inline, loop unroll, and block merge. Take this with a grain of salt since I'm not an expert of LLVM internals.

Submitting here because the LLVM folks already said they don't want this semantic. I'm not sure what the procedure for changing LLVM code is, so please let me know if I'm doing it wrong.

This updates the LLVM submodule to not optimize away calls when the
function might have an infinite loop.
@rust-highfive
Copy link
Collaborator

r? @cuviper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2020
@sfanxiang
Copy link
Contributor Author

An LLVM 8 test still causes non-terminate codegen test to fail. Keeping feature gate on for now.

@nagisa
Copy link
Member

nagisa commented Jun 21, 2020

Submitting here because the LLVM folks already said they don't want this semantic. I'm not sure what the procedure for changing LLVM code is, so please let me know if I'm doing it wrong.

This is unlikely to get merged here either for exactly this reason. We try to avoid having to carry a patch that is doomed to not be included upstream.

However, LLVM can delete a call to a function containing that loop.

This happens because a function like this:

define void @foo() #0 {
  br label %bb1

bb1:
  br label %bb1
}

is observed to not have any side effects and gains a readnone attribute indicating that foo is side-effect free. LLVM then can remove side-effect free function calls without having to look at the function body again.

Instead of considering all and any functions as side-effectful, not attaching a readnone attribute to those that contain a potentially infinite loop is likely to be cleaner and defeat fewer optimisations overall.

That said, that is also something LLVM decided they do not want. Instead the correct way forward is to invert the behaviour and adjust LLVM and clang's c++ generator as described here.

@sfanxiang
Copy link
Contributor Author

Instead the correct way forward is to invert the behaviour and adjust LLVM and clang's c++ generator

That would have exactly the same behavior on rust generated code, but I see your point :)

Hope this is still helpful for anyone who wants a workaround. Closing!

@sfanxiang sfanxiang closed this Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants