Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable support for default initializers for atomic fields (and possibly sync as well) #26268

Open
bradcray opened this issue Nov 19, 2024 · 1 comment

Comments

@bradcray
Copy link
Member

In https://github.com/Cray/chapel-private/issues/6871, it was noted that on someLocale var i: atomic int; doesn't currently work, where the reason is that we don't currently support compiler-generated default initializers for sync/atomic fields. That led me to refresh my memory about why we disallowed them, where I want to capture the notion that we should consider re-enabling support for atomic fields, and possibly for sync as well.

Reviewing the reasons for disallowing compiler-generated default initializers for these fields, I think there are two:

First reason

The first applies to sync and atomic fields, which is that in general for a field of type t:

record R {
  var x: t;
}

we generate an initializer that optionally takes an argument of type t as its type:

proc R.init(x: t = defaultValueOf(t)) {
  this.x = x;
}

However, for sync/atomic fields, this means that we'd take in a sync or atomic actual type and would have to apply a method to read it, e.g.:

record R2 {
  var x: atomic t;
  var y: sync t;
}

proc R2.init(x: atomic t = ..., y: sync t = …) {
  this.x = x.read();
  this.y = y.readXX();  // or maybe some other method?
}

I think the approach we should take here is to define the compiler-generated initializer arguments for sync t and atomic t fields simply be of type t since this is generally how such variables are initialized anyway (e.g., var x: atomic int = 1, y: atomic int = 2;). More to the point, we don't really support initializing a sync from another sync in user code (though we do for atomics, presumably for convenience and because there's no ambiguity as to the result?).

In this scheme, the initializer for the R2 record above would be:

record R2.init(x: t = defaultValueOf(t), y: t = defaultValueOf(t)) {
  this.x = x;
  this.y = y;
}

Second reason

The second reason is pretty specific to sync fields, and that is that we'd probably want to distinguish between whether the field should be initialized to be full or empty, potentially by distinguishing between the cases where a user passed in a value vs. didn't somehow. That is, given:

var s: sync int = 1,
    s2: sync int;

s will be initialized to be full, while s2 will be initialized to be empty. So presumably for a case like:

record R3 {
  var s: sync int;
}

we might expect the behavior of the following cases to be as indicated in the comments:

var myR = new R3(45),  // myR.s is full
   myR2 = new R3();    // myR.s is empty

Yet, to get this behavior, we'd either need to create multiple overloads of the initializer (2**(# of sync fields?)) or else introduce the ability to distinguish within Chapel code whether an argument got its value from its default value or used a user-supplied one (something that we, and users, have wanted anyway).

So this case also seems tractable, simply more work. Specifically, imagine something that logically worked like:

record R2.init(x: t = defaultValueOf(t), y: defaultValueOf(t)) {
  this.x = x;
  init this;
  if Reflection.userSupplied(y) then this.y = y;
}
@bradcray
Copy link
Member Author

For my tastes, I'd implement atomic fields in the near-term because it seems tractable, logical, and useful. The sync case requires more work, and doesn't seem quite as important to me, but would be nice to nail down eventually, particularly if it led us to introduce the ability to reason about whether the user passed an argument or relied upon its default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant