llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.8k stars 11.9k forks source link

`__restrict` pointers are not `indirectly_readable` #87178

Closed kedartal closed 2 months ago

kedartal commented 7 months ago

For example,

void f( double * __restrict ptr ) {
    using ptr_t = decltype(ptr);
    static_assert(!std::indirectly_readable<ptr_t>);
    static_assert(!std::is_same_v<std::remove_cv_t<ptr_t>, double *>);
}
concepts.h(127, 59): Because 'double *__restrict' does not satisfy 'indirectly_readable'
concepts.h(65, 31): Because 'remove_cvref_t<double *__restrict>' (aka 'double *__restrict') does not satisfy '__indirectly_readable_impl'
iterator_traits.h(519, 1): Because 'typename iter_value_t<_In>' would be invalid: no type named 'value_type' in 'std::indirectly_readable_traits<double *__restrict>'

(Godbolt)

This makes the standard std::advance and related functionality unavailable for such pointers (yet std::ranges::advance works fine, for anyone needing an immediate fix).

I think __restrict should be stripped out of the type, but instead it gets treated as if it were a const or volatile qualifier. Naturally, let's keep the no-aliasing optimization implications when lowering, just drop it from the c++ type, at least for class template arguments matching.

llvmbot commented 7 months ago

@llvm/issue-subscribers-clang-frontend

Author: Tal Kedar (kedartal)

For example, ```c++ void f( double * __restrict ptr ) { using ptr_t = decltype(ptr); static_assert(!std::indirectly_readable<ptr_t>); static_assert(!std::is_same_v<std::remove_cv_t<ptr_t>, double *>); } ``` ``` concepts.h(127, 59): Because 'const double *__restrict' does not satisfy 'indirectly_readable' concepts.h(65, 31): Because 'remove_cvref_t<const double *__restrict>' (aka 'const double *__restrict') does not satisfy '__indirectly_readable_impl' iterator_traits.h(519, 1): Because 'typename iter_value_t<_In>' would be invalid: no type named 'value_type' in 'std::indirectly_readable_traits<const double *__restrict>' ``` ([Godbolt](https://godbolt.org/z/61EM4crG6)) This makes the standard `std::advance` and related functionality unavailable for such pointers (yet `std::ranges::advance` works fine, for anyone needing an immediate fix). I think `__restrict` should be stripped out of the type, but instead it gets treated syntactically as if is were a `const` or `volatile` qualifier.
pinskia commented 7 months ago

Note GCC treats it the same way ...

kedartal commented 7 months ago

@pinskia Yup, I think both compilers employ the type system semantics of restrict in c also for c++ compilation units.

(just for the record, opened against clang version 19.0.0git (https://github.com/llvm/llvm-project.git a67b9326cd0448072a1192951f12f3927f31af8c))

kedartal commented 7 months ago

@ldionne I guess it's possible to circumvent the issue inside libc++ with something like remove_restrict (Godbolt) all over the place, but that's real yucky, and loses the no-aliasing semantics for optimization. Is there another way to do it purely in the library? Or perhaps issue a meaningful warning to the user?

pinskia commented 7 months ago

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45697 refences handling of restrict is slightly different between gcc and clang; Note I am not sure if that is still true though.

kedartal commented 7 months ago

@pinskia I don't know much about gcc. LLVM has some infrastructure to handle it in its IR (slides), but it's not fully implemented in the c++ frontend; I believe it's actually not entirely clear how to (e.g. what does restrict on a class member mean? AFAIK there's no consensus among the standard people). That's the real issue here, but I was wondering if there's a way to mask it in the standard library, until the frontend stuff is sorted out, to prevent the inconsistencies.

ldionne commented 6 months ago

IMO, it's likely that supporting __restrict as another cv-qualifier-like-thing consistently throughout the library is a non-starter. It's probably way too much work to do properly, since handling cv-qualifiers on types usually has to be considered during the design of most things in the library. I'm thinking about traits, concepts, but also other APIs. For example, should we overload some functions based on __restrict (if that's even possible) to behave slightly differently or be more efficient if we know there's no aliasing?

I could be blowing this out of proportion, but cv-qualifiers are fundamental to the type system and supporting a third kind of qualifier consistently seems like no small task to me. I'm happy to be convinced otherwise :)

kedartal commented 6 months ago

@ldionne My understanding is that restrict is not part of the type system in c++ (unlike c), so the frontend should have ignored it when doing any kind of template matching or overload selection; the no-aliasing semantics are orthogonal to the type aspects in either language (some IR attributes that in turn may impact lowering). Now, until this is what the frontend actually does for c++ sources, perhaps there's a way to temporarily make libc++ behave as if the frontend did the right thing -- by actively removing the qualifier, so the frontend never gets to see it in the first place. I think this is the opposite of what you wrote: not supporting the qualifier but "erasing" it, to mask the frontend issue.

ldionne commented 6 months ago

Even then, "erasing" it in all the required places for everything to work correctly is probably not realistic.

kedartal commented 6 months ago

@ldionne Completely agree when it comes to "everything", but it is a much smaller set if we just wanted to fix the situation for standard algorithms with raw pointers -- I guess that's the majority of real-world usage. If that's too much, then perhaps generating a more meaningful warning will help users understand the problem is related to restrict, not immediately obvious from the current no matching function for call to '__advance'.

philnik777 commented 6 months ago

The "just a bit" argument basically never works. The next person will come along and say "you've already added an extension for use case X, so please also do it for this small Y" and so on. Unless we have a rigorous description of where an extension should be applied and why, it's not going to happen. The set of use cases will grow until we have to basically write the library with it in mind, which is way too huge of a scope.

kedartal commented 6 months ago

@philnik777 Understood. I couldn't figure out who to talk to regarding fixing the underlying feature in the frontend -- I would appreciate any intro.

philnik777 commented 6 months ago

@AaronBallman @cor3ntin or @erichkeane are usually a good starting point.

kedartal commented 6 months ago

@AaronBallman @cor3ntin @erichkeane Do you think a patch that, in c++ mode, ignores the restrict (__restrict) qualifier during template argument matching and overload selection, is a good idea? Is there a more fundamental change that's more appropriate, e.g. remove it from the type entirely earlier in the pipeline (but then what about the no-aliasing semantics?), or maybe another approach? Should we move the discussion to somewhere other than this bug report?

erichkeane commented 5 months ago

I think that treating restrict as a qualifier is really the only sensible way to implement it, and I believe is effectively what the C spec requires. So implementing it differently in C++ would be awkward.

It seems to me that the library should need to support restrict like they do with CV qualifiers in order to make some of this stuff work.

I don't know if it makes sense to skip this during overload resolution, template instantiation/etc. I would expect that a restrict pointer and a non-restrict pointer should get different template instantiations... That said, Aaron is more of an expert than I am.

An RFC might be a best place to discuss this in front of the whole community though.

kedartal commented 5 months ago

@erichkeane Thanks! I guess you mean an RFC on the LLVM discourse forum? I'll try and create something over the next few days, after looking at some examples for other changes. I get your point about different implementation for c vs. c++, but until (if ever) the c++ standard includes restrict, the type systems are just not the same... your expectation regarding different template instantiation, to the best of my knowledge, does not currently conform with the c++ standard. The options covered so far are then:

erichkeane commented 5 months ago

@erichkeane Thanks! I guess you mean an RFC on the LLVM discourse forum? I'll try and create something over the next few days, after looking at some examples for other changes. I get your point about different implementation for c vs. c++, but until (if ever) the c++ standard includes restrict, the type systems are just not the same... your expectation regarding different template instantiation, to the best of my knowledge, does not currently conform with the c++ standard. The options covered so far are then:

* `restrict` as an LLVM extension, essentially adopting the c type system for c++ sources, with (trivial? difficult?) changes in libc++ to support it properly

* `restrict` gets ignored when it comes to type, so does not impact template instantiations and overload selection

* do nothing until we have a clearer idea where the c++ standard is heading

* (yucky) have libc++ cover up the inconsistency one way or another, without changes to the frontend

Yes, I mean an RFC on the LLVM discourse. As far as it being conforming: It would be a conforming extension to have it be separate template instantiations, and I think better reflects what happens in C.

Next, C++ will never add restrict to the standard. At least as long as I'm on the committee :) Defining the semantics of it in the more complicated C++ type system is, IMO, not particularly possible without some undesirable problems (the template instantiation problem is just one).

CVR qualifiers are in fact part of the 'type' in C, thus it makes sense to me that it would result in different types! That is static_assert(std::is_same_v<int, const int>); is a failed static assert, so should an equivilent with a restrict.

As far as the points above: It seems to make sense to me that if we're going to support restrict (in any form!) in C++, the library needs to consider it. It already has to do a remove_cv_ref type stuff all over the place, it seems to me that could become remove_cvr_ref.

cor3ntin commented 5 months ago

Unfortunately, I agree with both Erich and Louis.

It does not make senses to treat __restrict any differently than const in the core language. As it's part of declarations and types, it does not make sense to try to hack it out of overload resolution/deduction etc. And we would not want to deviate from C here.

On the flip side, supporting restrict in the library is an enormous ask. In recent years, we tend to go in the other direction, in that WG21 care less and less to support volatile qualifiers in the library, as supporting additional qualifiers everywhere has a combinatorial impact on everything, for very little value in most places (what even is a tuple<volatile int> ?)

So doing that work again, for a non-standard qualifier is... rather unlikely (and a humongous amount of work, to be clear). If the outcome of further discussion would be to keep the status quo, i really would not be surprised.

The semantics of restrict would probably be better expressed by something like a noalias attribute on functions or variables (as opposed to types), as it would avoid the virality concerns of the C qualifier approach.

We should also note that support for restrict in LLVM is still a work in progress AFAICT.

kedartal commented 5 months ago

Let's please move the discussion to the RFC.

ldionne commented 2 months ago

Since there doesn't seem to be a consensus for doing anything about this in https://discourse.llvm.org/t/rfc-supporting-restrict-in-libc-options, closing as NTBF for now. We can always reopen later if we reconsider this decision.