[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-10 Thread Sameer Sahasrabuddhe via cfe-commits
ssahasra wrote: Thanks for the example! I think #133684 is a correct incremental step forward, but maybe just going the whole way bottom-up is better. It is also correct to remove the verifier check. It's not a well-formedness error or even a semantic error to pass a convergence control token

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-10 Thread via cfe-commits
llvmbot wrote: @llvm/pr-subscribers-llvm-ir Author: Nathan Gauër (Keenuts) Changes When a callee is marked as `convergent`, some targets like HLSL/SPIR-V add a convergent token to the call. This is valid if both functions are marked as `convergent`. ADCE/BDCE and other DCE passes were al

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Sameer Sahasrabuddhe via cfe-commits
ssahasra wrote: To take this to its logical conclusion, when convergence tokens are in use, the `convergent` attribute is redundant. All we need is a `noconvergent` attribute for function declarations. A function definition is convergent iff the body contains a call to the `entry` intrinsic, a

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Matt Arsenault via cfe-commits
arsenm wrote: Thinking we should remove the IR verification requirement. It's more of a lint type check and violations would be UB https://github.com/llvm/llvm-project/pull/134844 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Nathan Gauër via cfe-commits
Keenuts wrote: miss-typed enter, sent the comment above too early. There are multiple ways to solve this issue: - fix the FunctionAttr to run DCE again once the `convergent` attribute is removed to make sure those invalid cases don't happen. - fix the FunctionAttr to not remove `convergent` i

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Nathan Gauër via cfe-commits
Keenuts wrote: FYI: there is this PR which I think will replace this one: https://github.com/llvm/llvm-project/pull/134863 > I didn't understand the validity part. Why is the caller required to be > convergent in order to add a token to a callsite? Given this example: ```llvm declare i32 @fo

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Sameer Sahasrabuddhe via cfe-commits
ssahasra wrote: > When a callee is marked as convergent, some targets like HLSL/SPIR-V add a > convergent token to the call. This is valid if both functions are marked as convergent. I didn't understand the validity part. Why is the caller required to be convergent in order to add a token to a

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-09 Thread Sameer Sahasrabuddhe via cfe-commits
ssahasra wrote: >From the spec for convergence control tokens: https://llvm.org/docs/ConvergentOperations.html#inferring-non-convergence > An optimizer may remove the convergent attribute on a function if it can > prove that the function does not contain a call to > `llvm.experimental.converge

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Matt Arsenault via cfe-commits
arsenm wrote: > > Turns out not really, I ran spec with this about 2 years ago and the only > > non-noise change was a mild improvement > > Looking at the PR you linked, seems like there was still not a clear > consensus on the default change no? Yes, there were people who are wrong and need

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Nathan Gauër via cfe-commits
Keenuts wrote: > Turns out not really, I ran spec with this about 2 years ago and the only > non-noise change was a mild improvement Looking at the PR you linked, seems like there was still not a clear consensus on the default change no? (And I'd assume consumers like llvm-translator won't be

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Matt Arsenault via cfe-commits
arsenm wrote: > I suspect the long-term change to change the default IR to assume convergent > will take some time as it will impact many subprojects. Turns out not really, I ran spec with this about 2 years ago and the only non-noise change was a mild improvement > Would you be OK with me

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Nathan Gauër via cfe-commits
Keenuts wrote: > These should not have side effects. I agree I'm stretching the "hasSideEffect" definition here. > Whatever is removing the convergent needs to consider the uses (or more > likely, just leave it alone) I suspect the long-term change to change the default IR to assume convergen

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Matt Arsenault via cfe-commits
https://github.com/arsenm requested changes to this pull request. These should not have side effects. The problem you are experiencing is because the design of the convergent attribute is broken. It imposes a structural requirement on the IR, rather than relaxing an assumed restriction. Whateve

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread via cfe-commits
llvmbot wrote: @llvm/pr-subscribers-backend-spir-v Author: Nathan Gauër (Keenuts) Changes When a callee is marked as `convergent`, some targets like HLSL/SPIR-V add a convergent token to the call. This is valid if both functions are marked as `convergent`. ADCE/BDCE and other DCE passes

[clang] [llvm] [IR] Mark convergence intrins as has-side-effect (PR #134844)

2025-04-08 Thread Nathan Gauër via cfe-commits
https://github.com/Keenuts created https://github.com/llvm/llvm-project/pull/134844 When a callee is marked as `convergent`, some targets like HLSL/SPIR-V add a convergent token to the call. This is valid if both functions are marked as `convergent`. ADCE/BDCE and other DCE passes were allowed