Skip to content

Conversation

@jchlanda
Copy link
Contributor

This patch attempts to generalise the workaround: #1334
The problem is as follows:

if (T0)
  create/store variable for the use by all the threads
barrier()
use variable/load

LLVM doesn't know that barrier (__syncthreads) interacts with the store/load sequence. The offending spot is here: https://git.office.codeplay.com/oneapi-core/intel-llvm-mirror/-/blob/sycl/llvm/lib/Analysis/GlobalsModRef.cpp#L536 it is assumed that all intrinsics are save to process (even though they have hasSideEffects set by default). The pass in question works on llvm::Function declaration and the only available information is:

; Function Attrs: convergent nounwind
declare void @llvm.nvvm.barrier0() #3
...
attributes #3 = { convergent nounwind }

This patch adds intrinsic property that is propagated all the way up to function attribute, modifying the declaration of the barrier0 to:

...
attributes #3 = { convergent disjoint_agents nounwind }

something that the pass can then check against.

The original bug report: #1258

@jchlanda jchlanda requested a review from bader as a code owner October 26, 2021 13:49
@bader bader requested a review from againull October 26, 2021 14:27
@againull
Copy link
Contributor

Thank you for working on this issue! Should we consider opening a PR in llvm-project repo?

@jchlanda
Copy link
Contributor Author

Thank you for working on this issue! Should we consider opening a PR in llvm-project repo?

We discussed it and came to a conclusion that first we should consult the wider community and gather their opinion on the problem/solution. Codeplay will send request to the mailing list to address that.

@bader
Copy link
Contributor

bader commented Mar 3, 2022

@jchlanda, can we close this PR?

@jchlanda
Copy link
Contributor Author

jchlanda commented Mar 3, 2022

Yeap, closing now.

@jchlanda jchlanda closed this Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants