[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)

via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 25 19:01:25 PDT 2024


Sirraide wrote:

> Have you had a chat with OpenMP folks?

@alexey-bataev has been commenting on this matter since the original assume pr, and according to them, `[[omp::assume]]` is the only spelling of the attribute mandated by the OpenMP standard, and we’ve already added that one in #84582. All the other spellings were apparently never really part of OpenMP to begin with.

> The logic would be that openmp would use something like `[[omp::assume]]` as the non-namespaced attributes are only supposed to be used by the C & C++ standards and `[[clang::assume]]` is, as you point out, horribly confusing. But this would need buy-in from openmp. `[[clang::omp_assume]]` is _fine_ but we are inventing yet more things out of spec, which is not thrilling. `[[omp_assume]]` would be fine too, but we should avoid having different spellings in C and C++

I don’t like `omp_assume` either; I’ve only added it because there’s one header in LibCL that uses `__attribute__((assume))`, and I’m candidly just not familiar enough with OpenCL to know whether that can just be replaced with `[[omp::assume]]`; if that’s possible, then I’d definitely not even add `omp_assume` to begin with. It’s is only there as a last resort in case that OpenCL header really can’t do w/o the `__attribute__` spelling—that way I can simply remove it again before we merge this if it turns out that we don’t need it.

> And we should be careful about breaks, hence why I prefer a deprecation approach. We should avoid `[[clang::assume]]` change meaning in the same version.

It wouldn’t really change meaning as there is still a difference in appertainment between the C++ attribute and the OpenMP attribute—unless you mean that it would change from just working to issuing a warning/error when applied to a function, in which case, yeah, we might want to deprecate it first. The only reason I’m considering removal as a possibility is because I don’t think anyone is actually using this spelling, from what I can tell—but of course, there may be users that we’re just not aware of...

https://github.com/llvm/llvm-project/pull/84934


More information about the cfe-commits mailing list