Allow global_ptr<const T> w/ implicit conversions

[Dan Bonachea]

There may be some documentation value to providing a global_ptr_const<T>, with implicit conversion from global_ptr<T> (but not the other way around). The upcast and downcast would consume/produce a (const T *) instead of a (T *).

This would allow functions like rget to take a global_ptr_const<T> src argument, documenting the fact the source memory is not modified by the callee.

Similarly, interfaces within user applications that pass global_ptr<T>'s around could use this feature to document which global pointers treat the target object as const.

[Dan Bonachea]

Alternatively, we could provide implicit conversion from global_ptr<T> to global_ptr<T const> (assuming that's technically possible in the template magic) and use the latter in APIs like rget source address.

[Amir Kamil]

I've run into major problems with template deduction when it comes to global_ptr<const T>, even with implicit casts available. For example:

template<typename T>
void rget(global_ptr<const T> src) {
}

template<typename T>
void rget(global_ptr<const T> src, T *dst, size_t count) {
}

int main() {
  double *p1 = new double(3.14);
  global_ptr<double> g1(p1);
  rget(g1);
  rget(g1, p1, 1);
}

This results in failure to deduce:

foo.cpp:72:3: error: no matching function for call to 'rget'
  rget(g1);
  ^~~~
foo.cpp:46:6: note: candidate template ignored: cannot deduce a type for 'T'
      that would make 'const T' equal 'double'
void rget(global_ptr<const T> src) {
     ^
foo.cpp:50:6: note: candidate function template not viable: requires 3
      arguments, but 1 was provided
void rget(global_ptr<const T> src, T *dst, size_t count) {
     ^
foo.cpp:73:3: error: no matching function for call to 'rget'
  rget(g1, p1, 1);
  ^~~~
foo.cpp:50:6: note: candidate template ignored: cannot deduce a type for 'T'
      that would make 'const T' equal 'double'
void rget(global_ptr<const T> src, T *dst, size_t count) {
     ^
foo.cpp:46:6: note: candidate function template not viable: requires single
      argument 'src', but 3 arguments were provided
void rget(global_ptr<const T> src) {
     ^
2 errors generated.

This means that we cannot modify the API for rput/rget to take in global_ptr<const T>.

An alternative would be to put std::remove_const_t<T> in appropriate places so that the API works with both global_ptr<T> and global_ptr<const T>, and the compiler deduces the appropriate substitution. However, it's possible that this may cause unintended problems, so I think we should defer this until after the spec release so that we have an opportunity to work through the issues.

Since I don't want to encourage use of global_ptr<const T> until we figure out if we can properly support it, I think we should defer adding conversions to the spec as well. But they are rather straightforward to implement:

  global_ptr(const global_ptr<std::remove_const_t<T>> &other) :
    raw_ptr(other.raw_ptr) {}
  global_ptr<std::remove_const_t<T>> remove_const() const {
    return global_ptr<std::remove_const_t<T>>(const_cast<std::remove_const_t<T>*>(raw_ptr));
  }

Or instead of the constructor:

  operator global_ptr<std::add_const_t<T>>() const {
    return global_ptr<std::add_const_t<T>>(raw_ptr);
  }

[Dan Bonachea]

Thanks for looking into this Amir.

Do the instantiation problems go away if one uses the (admittedly uglier) syntax of a different class (eg. global_ptr_const<T>) with the appropriate implicit conversion?

[Amir Kamil]

Unfortunately, no:

foo.cpp:77:3: error: no matching function for call to 'rget'
  rget(g1);
  ^~~~
foo.cpp:51:6: note: candidate template ignored: could not match
      'global_ptr_const' against 'global_ptr'
void rget(global_ptr_const<T> src) {
     ^
foo.cpp:55:6: note: candidate function template not viable: requires 3
      arguments, but 1 was provided
void rget(global_ptr_const<T> src, T *dst, size_t count) {
     ^
foo.cpp:78:3: error: no matching function for call to 'rget'
  rget(g1, p1, 1);
  ^~~~
foo.cpp:55:6: note: candidate template ignored: could not match
      'global_ptr_const' against 'global_ptr'
void rget(global_ptr_const<T> src, T *dst, size_t count) {
     ^
foo.cpp:51:6: note: candidate function template not viable: requires single
      argument 'src', but 3 arguments were provided
void rget(global_ptr_const<T> src) {
     ^
2 errors generated.

The culprit in both cases appears to be the following (http://en.cppreference.com/w/cpp/language/template_argument_deduction):

Type deduction does not consider implicit conversions (other than type adjustments listed above): that's the job for overload resolution, which happens later.

[BrianS]

but you wouldn't do an rget to a const T

you would do an rput, in which case the input T is const

Brian

[Amir Kamil]

The const pointer is being used as the source of the rget, not the destination. rput does not suffer from this problem since the source must be a local pointer and the destination a non-const global pointer.

[BrianS]

namespace upcxx
{
  template <class T>
  class global_ptr
  {
  public:
    global_ptr(T* ptr): m_ptr(ptr) {;}
    T* local(){return m_ptr;}
    operator global_ptr<const T>() {return *this;}
  private:
    T* m_ptr;
  };

  template <class T, typename... Args> 
  global_ptr<T> new_(Args&&... args)
  {
    return global_ptr<T>(new T(std::forward<Args>(args)...));
  };


  template<typename T>
  void rget(global_ptr<const T> src) {   }

  template<typename T>
  void rget(global_ptr<const T> src, T *dst, size_t count) { }
}



int main(int argc, char* argv[])
{
  double *p1 = new double(3.14);
  upcxx::global_ptr<double> g(p1);
  upcxx::global_ptr<const double> g1 = g; 
  upcxx::rget(g1);
  upcxx::rget(g1, p1, 1);

[Amir Kamil]

I'm not in favor of requiring explicit conversion or explicit function template specialization. I think that significantly detracts from the user experience.

[BrianS]

we can overload rget

namespace upcxx
{
  template <class T>
  class global_ptr
  {
  public:
    global_ptr(T* ptr): m_ptr(ptr) {;}
    T* local(){return m_ptr;}
    operator global_ptr<const T>() {return *this;}
  private:
    T* m_ptr;
  };

  template <class T, typename... Args> 
  global_ptr<T> new_(Args&&... args)
  {
    return global_ptr<T>(new T(std::forward<Args>(args)...));
  };


  template<typename T>
  void rget(global_ptr<const T> src) {   }

  template<typename T>
  void rget(global_ptr<T> src) { }

  template<typename T>
  void rget(global_ptr<const T> src, T *dst, size_t count) { }
}



int main(int argc, char* argv[])
{
  double *p1 = new double(3.14);
  upcxx::global_ptr<double> g(p1);
  upcxx::global_ptr<const double> g1 = g; 
  upcxx::rget(g);
  upcxx::rget(g1, p1, 1);
  return 0;
}

if your remote rank shares a global_ptr<const T> with other ranks, they get the const version. If the remote rank shares the non-const version, that works too.

[Dan Bonachea]

Brian - your example might compile, but it's not the usage case I'm asking about.

As a user, I'd like the ability to pass either global_ptr<const double> or global_ptr<double> directly to the source argument of an rget() call (with no explicit conversion or casting), in exactly the same way that I can pass either const double * or double * directly to the source address of a void *memcpy(void * dst, const void * src, size_t n); call. Ie the one-way implicit conversion of T * to const T * based on formal argument type that takes place in function call semantics.

It's possible that defining two overloads for each version of rget (with and without const) can accomplish what I want for that particular function. However if that's the only solution we can offer, then I would see that as a deficiency of our pointer-like class (ie not really practical if I'm building an interface atop UPC++ that takes 8 global_ptr<T>s, any of which may be const).

[Amir Kamil]

I agree that providing/requiring an exponential number of overloads is not a viable solution.

With the current API definition, you can actually pass a global_ptr<const double> as an argument. It deduces T = const double, and the other arguments must be compatible with this deduction. This means that we currently permit using a type like global_ptr<const double> as the target of an rput/rget. This is one reason I really don't want to explicitly support global_ptr<const T> yet, as we would then be responsible for prohibiting operations like this.

We do potentially have a viable path forward using std::remove_const as I suggested above. This would allow the compiler to deduce either T = const double or T = double as appropriate. However, as this would be a pervasive change, I don't think we should do it until we can guarantee it won't come back to bite us. We need to make sure that a change does not introduce ambiguities in overload resolution, and that all intended use cases compile correctly. So I think we should defer this to after the spec release.

[Dan Bonachea]

This means that we currently permit using a type like global_ptr<const double> as the target of an rput

Good catch - seems the preconditions for rput/rget should state T is not const-qualified, and the implementation should probably static_assert(!std::is_same<T, T const>);

[BrianS]

We will have to support some version of this.

If a user declares their data as global_ptr<const T> and we have no const T versions, then the symmetric functions fail to compile

namespace upcxx
{
  template <class T>
  class global_ptr
  {
  public:
    global_ptr(T* ptr): m_ptr(ptr) {;}
    T* local(){return m_ptr;}
    operator global_ptr<const T>() {return *this;}
  private:
    T* m_ptr;
  };

  template<typename T>
  void rget(global_ptr<T> src) { }

  template<typename T>
  void rget(global_ptr<T> src, T *dst, size_t count) { }
}



int main(int argc, char* argv[])
{
  double *p1 = new double(3.14);
  upcxx::global_ptr<double> g(p1);
  upcxx::global_ptr<const double> g1 = g; 
  upcxx::rget(g);  //yup, compiler happy.
  upcxx::rget(g1, p1, 1);   //This fails to compile now.  I can't get a remote const.
  return 0;
}

The user made a pointer to a const value. They expect remote ranks to respect that.

I still advocate user space casting. Our API will then be correct and not combinatoric at the cost of users needing to take the correct steps. We can provide the C++ short cutting in the next version. const vs non-const is something a user resolves during bootstrapping, and they will want their contracts respected. It breaks contracts to make all rput/rget abandon const-ness. The simplest SPMD model is Owner Computes. The rank that owns the data has the contract to mutate it. remote ranks fill their "ghost" state from their remote ranks through const data access. Then order is irrelevant. If I have to worry about the value of my remote read depending on when the reads are serviced by the Owner then that is another range of bugs to worry about.

I'm not making things easier here, but I did think about it. If I can let the compiler enforce rget is const, then that helps.

[Scott Baden]

Brian and Amir, Are you in agreement with Brian's rationale?

Scott

[Dan Bonachea]

Here's another idea - what if we tweaked the current declaration of rget:

template <typename T>
future <> rget ( global_ptr <T> src , T *dest , std::size_t count );

to this:

template <typename TS, typename TD>
future <> rget ( global_ptr <TS> src , TD *dest , std::size_t count );

and in the preconditions require:

1. TD and TS must be qualified or unqualified versions of the same type
2. TD must not be const-qualified

By separating the template type parameters used for source and destination memory, this should hopefully allow instantiation with TD=(double) and either TS=(const double) or TS=(double), with a single overload and no extra casts or conversions by the user.

Here's a sample implementation that works as expected for me (using g++ 5.4.0 -std=c++11):

#include <cstdlib>
#include <type_traits>
namespace upcxx {
  template <class T>
  class global_ptr
  {
  public:
    global_ptr(T* ptr): m_ptr(ptr) {;}
    T* local(){return m_ptr;}
    operator global_ptr<const T>() {return *this;}
  private:
    T* m_ptr;
  };

template <typename TS, typename TD>
void rget ( global_ptr <TS> src , TD *dest , std::size_t count ) {
  static_assert(std::is_same<TD const volatile, TS const volatile>::value,
                "TD and TS must be cv-qualified versions of the same type");
  static_assert(!std::is_same<TD, TD const>::value, 
                "TD may not be const-qualified");
}
}

int main(int argc, char* argv[]) {
  double *p1 = new double(3.14);
  const double *p2 = p1;
  upcxx::global_ptr<double> g(p1);
  upcxx::global_ptr<const double> g1 = g; 
  upcxx::rget(g, p1, 1);   // works 
  upcxx::rget(g1, p1, 1);  // works
 #if 0 // error cases:
  upcxx::rget(g, p2, 1);   // const-violation error, as intended
  upcxx::rget(g1, p2, 1);   // const-violation error, as intended
  upcxx::rget(g1, &argc, 1);   // type mismatch error, as intended
 #endif
  return 0;
}

[Amir Kamil]

I agree that we should support cv qualification eventually, but I'm not sure we should tackle this before the spec release. However, if others feel strongly that we should, I can go ahead and do that.

cv qualification is not just an issue for rput/rget. Here are a proposed set of changes:

• In future<T...> and promise<T...>, the types in T... should be prohibited from being cv qualified.
• In global_ptr<T>, T should be prohibited from being qualified with volatile. (We may want to relax this at some point, but I think this restriction will make the typing simpler for rget that works with futures or promises.)
• The target of rput/rget/atomics/VIS should be prohibited from being const qualified.
• All communication ops that work with futures and promises should work with const-erased types (e.g. using std::remove_const). This includes collectives.

I'm not sure what to do with atomic_get(). Should it be legal to call on a global_ptr<const T>?

[Dan Bonachea]

Here are a proposed set of changes:

In future<T...> and promise<T...>, the types in T... should be prohibited from being cv qualified.

Explain why? Qualifiers are have no semantic impact on rvalues, so they seem mostly harmless/meaningless here. A result value stored in a future or promise is usually an internal copy of the original anyhow. We already discourage users from mutating the internal value stored in a promise, if they choose to put a const on their type that just helps prevent that dubious practice.

In global_ptr<T>, T should be prohibited from being qualified with volatile. (We may want to relax this at some point, but I think this restriction will make the typing simpler for rget that works with futures or promises.)

volatile is probably of no use to UPC++ programmers, who should be using atomic/rpc/lpc to manage memory consistency and concurrency, so I concur.

The target of rput/rget/atomics/VIS should be prohibited from being const qualified.

Definitely. And the source should allow (but not require) const qualification. As in my example above.

All communication ops that work with futures and promises should work with const-erased types (e.g. using std::remove_const). This includes collectives.

I agree this should happen internally. Not sure if you are proposing an interface change here.

I'm not sure what to do with atomic_get(). Should it be legal to call on a global_ptr<const T>?

I'd say yes, absolutely. A read (even with the strongest possible memory model settings) does not violate the contract of const.

[Amir Kamil]

Explain why? Qualifiers are have no semantic impact on rvalues, so they seem mostly harmless/meaningless here. A result value stored in a future or promise is usually an internal copy of the original anyhow. We already discourage users from mutating the internal value stored in a promise, if they choose to put a const on their type that just helps prevent that dubious practice.

Promise fulfillment modifies the result value. To me, it does not make semantic sense to have a promise<const T>, since that implies that the promise can never be fulfilled. Prohibiting cv qualification also will simplify the typing rules, in the same way that prohibiting volatile on global pointers does.

I agree this should happen internally. Not sure if you are proposing an interface change here.

In cases where a return type is future<T>, we will definitely need to change the return type to future<std::remove_const_t<T>>, since return type does not participate in template deduction. Example: calling rget on a global_ptr<const double> should produce a future<double>, not a future<const double>. In other cases, such as when a promise is taken in as an argument, we can either use std::remove_const or introduce a new template parameter as you suggested.

[Dan Bonachea]

To me, it does not make semantic sense to have a promise<const T>, since that implies that the promise can never be fulfilled.

I agree for a promise<T> const, since that can never be fulfilled.

I think the const qualifier on promise<T const> is effectively meaningless, and in any case I think you could make a convincing argument that fulfill_promise is the required/unique variable initializer for the stored const result. That being said, I could be convinced to prohibit as you suggest, provided that prohibition doesn't force users to const-erase in awkward ways when instantiating futures and promises, even though there's no possible breach of the const contract here.

In cases where a return type is future<T>, we will definitely need to change the return type to future<std::remove_const_t<T>>, since return type does not participate in template deduction.

I see what you mean - yes I agree.

[Amir Kamil]

The only situation I can think of where cv-erasing is required is when naming a future<T> or promise<T> where T is a template argument that is deduced to be cv qualified. However, even if we were to allow cv qualification, I suspect most cases like this would break anyway without cv erasure if we don't support const conversions for futures/promises.

[BrianS]

for global_ptr<const T> the exact same lack of automatic conversion happens with std::shared_ptr A one line assignment from non-const to const is a standard C++ idiom followed by the pointer managing abstractions in the language and automagically finding the conversion results in other kinds of ambiguous resolution that the C++ standards committees were unwilling to unravel.

allocate.cpp:41:3: error: no matching function for call to 'sget'
  upcxx::sget(d);
  ^~~~~~~~~~~
allocate.cpp:26:8: note: candidate template ignored: cannot deduce a type for
      'T' that would make 'const T' equal 'double'
  void sget(std::shared_ptr<const T> src) {  }

[BrianS]

I will ask some of the C++ Committee folks about this when I'm at the ISO committee meeting in two weeks to get a definitive resolution.

[Dan Bonachea]

There's another interesting approach to this problem explained here.

In a nutshell, it globally provides the desired implicit ptr<T> to ptr<const T> conversion through inheritance on the pointer-wrapper type.

[Amir Kamil]

For now, we are prohibiting cv qualification on T. This was added in 1845f19.

[BrianS]

I think the stackoverflow trick Dan looked into will work. I coded up a global_ptr based on this and did rget and rput with the right qualifiers. It held up.

So, after we pop the ECP administrative deadlines I'd like to re-open this one right away.

[Amir Kamil]

A major problem with the inheritance approach is that it allows the const qualifier to be cast away by a standard static or dynamic cast. So I would advocate for a different approach.

[Dan Bonachea]

Amir - such explicit casts are of course also possible for normal pointers using const_cast. Code that explicitly casts away a const is intentionally breaking the contract and asking for undefined behavior, but that's not statically prohibited (nor should it be). Your objection is that under this approach such a potentially-dangerous cast would not be spelled "const_cast"?

I agree it's a wart, but it still seems the most self-contained solution we've found so far (ie that handles constness as part of the global_ptr class itself, instead of requiring explicit goop in every single client using global_ptr in an interface).

[Amir Kamil]

I suppose the static cast is only an issue when working on a global_ptr through a pointer or reference. That may not be a common enough case to worry about.

Still, I would like to get Bryce's opinion on this. We may be missing some other reason why the standard does not use this pattern for shared_ptr.

[BrianS]

So I have tried out a test of the inheritance trick as outlined in stackoverflow where you use the idea the X<T> IS A X<const T> from a functionality perspective. Hence, you can tell the compiler that this is a legal cast. Working out how to do all this in our January delivery time is a tough deadline though. Updated example with actual access protections worked out correctly, the base class provides the const T* access, while the derived non-const version provides the modifying version. Somehow this feels like it is violating encapsulation, since I overload a const function in the derived class.

#include <cstddef>

template<typename T>
struct future
{
  T m_value;
  bool m_ready=false;
}; 

template<typename T>
struct global_ptr : global_ptr<const T>
{
  using global_ptr<const T>::global_ptr;
  T* ptr()  {return *this->m_value;}
}; 

template<typename T>
struct global_ptr<const T>
{
  explicit global_ptr(T* val):m_value(val){}
  T* ptr() const {return m_value;}
protected:
  T* m_value=nullptr;
}; 

template<typename T>
future<T> rget(global_ptr<const T> src) {
  future<T> rtn;
  rtn.m_value=*(src.ptr());
  rtn.m_ready=true;
  return rtn;  
}

template<typename T>
void rget(global_ptr<const T> src, T *dst, std::size_t count) {
  for(std::size_t i=0; i<count; ++i) dst[i]=src.ptr()[i];
}

int main() {
  double *p1 = new double(3.14);
  global_ptr<double> g1(p1);
  global_ptr<const double> g2(p1);
  auto a = rget(g1);
  auto b = rget(g2);
  rget(g1, p1, 1);
  rget(g2, p1, 1);
}

[Dan Bonachea]

Thanks @bvstraalen - this prototype seems like a good argument that with reasonable effort we can make this work in a way that will behave naturally for end users.

Any strong objections?

[BrianS]

Well, I'm just getting a better grip on the class-scope using command. I think I grock it now. It is nice in that you don' t have the compiler whining about a derived class hiding functions, you can tell it that's your plan. It should also always be ok to cast to const. We can run through this one in the meeting and assign it to me.

[Dan Bonachea]

This issue was discussed in the 1/10/18 meeting and Brian agreed to investigate the spec and implementation for March release.

[Dan Bonachea]

In the 2018-03-09 meeting we resolved to defer resolution of this issue to next release.

[Dan Bonachea]

This issue was triaged at the 2018-06-13 Pagoda meeting and assigned a new milestone/priority.

[Amir Kamil]

Fixed by PR #42.