split-init for out intent and folded param conditional code

[mppf]

PR #14917 adds support for split-init for out intent and later split init decisions in function resolution. However certain patterns currently rely on param-folding, which is different from other split-init. It is arguably an implementation artifact.

For example, the following two tests behave differently:

record R {
  var x: int;
  proc init=(other: R) {
    writeln("copy init");
  }
}
operator =(ref lhs: R, rhs: R) {
  writeln("assign");
}
proc makeR(arg:int) {
  return new R(arg);
}

proc outFn(out arg: R) {
  arg = makeR(1);
}

config param paramOption = false;
config const constOption = false;

proc test1() {
  writeln("test1");
  var x: R;
  if paramOption {
    writeln(x);
  }
  outFn(x);
}
test1();

proc test2() {
  writeln("test2");
  var x: R;
  if constOption {
    writeln(x);
  }
  outFn(x);
}
test2();

Here is another example, where the analysis of return patterns within the functions differs after param folding has occurred. These variants also behave differently:

record R {
  var x: int;
  proc init() {
    writeln("default init");
  }
  proc init(arg: int) {
    writeln("init ", arg);
    this.x = arg;
  }
  proc deinit() {
    writeln("deinit");
  }
  proc init=(other: R) {
    writeln("copy init");
  }
}
operator =(ref lhs: R, rhs: R) {
  writeln("assign");
}
proc makeR(arg:int) {
  return new R(arg);
}

config param paramOption = false;
config const constOption = false;

proc outFn1(out arg: R) {
  if paramOption then
    return;
  arg = makeR(1);
}

proc test1() {
  writeln("test1");
  var x: R;
  outFn1(x);
  writeln(x);
}
test1();

proc outFn2(out arg: R) {
  if !paramOption then
    return;
  arg = makeR(1);
}

proc test2() {
  writeln("test2");
  var x: R;
  outFn2(x);
  writeln(x);
}
test2();

proc outFn3(out arg: R) {
  return;
  arg = makeR(1);
}

proc test3() {
  writeln("test3");
  var x: R;
  outFn3(x);
  writeln(x);
}
test3();

proc outFn4(out arg: R) {
  if constOption then
    return;
  arg = makeR(1);
}

proc test4() {
  writeln("test4");
  var x: R;
  outFn4(x);
  writeln(x);
}
test4();

proc outFn5(out arg: R) {
  if !constOption then
    return;
  arg = makeR(1);
}

proc test5() {
  writeln("test5");
  var x: R;
  outFn4(x);
  writeln(x);
}
test5();

We would like the variants in these examples to have the same behavior as far as split-init.

Associated tests:

test/types/records/intents/issue-14996-1.chpl test/types/records/intents/issue-14996-2.chpl test/types/records/intents/issue-14996-3.chpl

[bradcray]

@mppf: This is another one that caught my eye while surfing through issues due to split-init and out being mentioned. The first case seems to be generating the same output for both tests, making me think the situation has improved (?). The second I didn't take the time to understand the expected behavior figuring you'd be more familiar with the original problem and the current state of things.

[mppf]

PR #18593 adds tests for these cases so we can be aware of any change in behavior.

I think the question is - should split init always happen before param folding (param folding away of conditionals)? Or should it always happen after?

Today, what we have is a mix, where the split init for out intent formals is handled after param folding (test1 / test2 in the second example -- see https://github.com/chapel-lang/chapel/blob/main/test/types/records/intents/issue-14996-3.chpl , but split init for regular variables is handled before param folding (test1 / test2 in the first example -- see https://github.com/chapel-lang/chapel/blob/main/test/types/records/intents/issue-14996-1.chpl).

[mppf]

I think it’s not so nice for it to do split init with out one way and with = a different way. To improve that, we would need to either consider the param conditionals folded, or always consider them not folded. But, we can’t process split init with out unless we resolve the branch of the conditional (because we won’t know which function is called at all if we don’t resolve it — so we don’t know that a function with out intent was called). As a result, I think we have to choose one of:

1. Keep it inconsistent between split init with = and split init with an out formal
2. Make both split init with = and split init with an out formal happen “after param folding” that is, ignore param false branches
3. Require some sort of syntactic marker on actuals passed to out intent formals (or those that are used to split init), e.g. writing a call like myFnWithOut(out actual) (Q: how would that combine with named argument passing?)

[bradcray]

I think it’s not so nice for it to do split init with out one way and with = a different way.

I agree with this.

As a result, I think we have to choose one of:

option 2 is most attractive to me if it's tractable (in the sense that it's what I found myself wanting intuitively before reading the options). Is it?

[DanilaFe]

Option 2 seems most intuitive to me as well; I think Michael is concerned about the ability to tell if split-init is happening at a glance, however (the more complicated the logic that happens before we decide to split init or not, the harder it is to tell just by looking at a program).

[mppf]

Hmm, the first case from the original post seems to behave the same for test1 and test2 on main today (prints out assign twice). Ditto for the second post.

Similarly, this program shows that out and = assignment behave the same with respect to a conditional that could be param-folded out (prints out assign twice)

record R {
  proc init=(other: R) { writeln("copy init"); }
}
operator =(ref lhs: R, rhs: R) { writeln("assign"); }

proc splitInitAssign() {
  var x: R; 
  if false {
    writeln(x);
  }
  x = new R();
}
splitInitAssign();

proc outFn(out arg: R) {
  arg = new R();
}

proc splitInitOut() {
  var x: R;
  if false {
    writeln(x);
  }
  outFn(x);
}
splitInitOut();

I think that the compiler must be relying on tracking mentions of variables in folded-away code for this to work. So this is sortof Option 4 since I didn't list it in my options.

But, https://github.com/chapel-lang/chapel/blob/main/test/types/records/intents/issue-14996-3.chpl shows a case where the out intent handling is relying on param folding:

proc outFn1(out arg: R) {
  if false then
    return;
  arg = makeR(1);
}

Here arg is split-inited, but it wouldn't be legal to split-init it if we were imagining the analysis happens before param-folding.

So, I think that make the problem described in this issue more subtle (and arguably bug-like than) than it sounded like above.

Nonetheless, we can consider moving the whole split-init project to happen after param folding, if we see benefits to do that. But I am not sure such a change is well-motivated at this point.

[mppf]

I keep thinking "but wait that can't possible work" but then I have trouble coming up with an example where it is broken.

Here is an attempt that works as I would expect (with the current design)

``` chapel record R { var x = 0; proc init() { writeln("default init"); } proc init(arg: int) { writeln("init ", arg); this.x = arg; } proc init=(other: R) { writeln("copy init"); } } operator =(ref lhs: R, rhs: R) { writeln("assign"); } proc fOut(out arg: R) { arg = new R(1); } config param paramOption = false; proc testA() { writeln("testA"); var x: R; if paramOption { x = new R(1); } else { x = new R(1); } writeln(x); } testA(); proc testB() { writeln("testB"); var x: R; if paramOption { fOut(x); } else { fOut(x); } writeln(x); } testB(); proc testC() { writeln("testC"); var x: R; if paramOption { x = new R(1); } else { fOut(x); } writeln(x); } testC(); proc testD() { writeln("testD"); var x: R; if paramOption { fOut(x); } else { x = new R(1); } writeln(x); } testD(); ```

Here is a case where it's not always working the way I would expect. Here I was expecting that, if split init is processed before param folding, that we would decide that we can't split-init x, because then part mentions x without initializing it. (But, I'm not seeing a difference between out and = in this one either). Interestingly, in this case, we decide not to split-init x in the early passes, but the decide that we should split-init it during resolution.

The output of this test indicates that x is split-inited:

config param paramOption = false;

proc testA() {
  writeln("testA");
  var x: R;
  if paramOption {
    x;
  } else {
    x = new R(1);
  }
  writeln(x);
}
testA();

(full source code in details below).

``` chapel record R { var x = 0; proc init() { writeln("default init"); } proc init(arg: int) { writeln("init ", arg); this.x = arg; } proc init=(other: R) { writeln("copy init"); } } operator =(ref lhs: R, rhs: R) { writeln("assign"); } proc fOut(out arg: R) { arg = new R(1); } config param paramOption = false; proc testA() { writeln("testA"); var x: R; if paramOption { x; } else { x = new R(1); } writeln(x); } testA(); proc testB() { writeln("testB"); var x: R; if paramOption { x; } else { fOut(x); } writeln(x); } testB(); proc testC() { writeln("testC"); var x: R; if paramOption { x = new R(1); } else { x; } writeln(x); } testC(); proc testD() { writeln("testD"); var x: R; if paramOption { fOut(x); } else { x; } writeln(x); } testD(); ```

[bradcray]

Hmm, the first case from the original post seems to behave the same for test1 and test2 on main today (prints out assign twice).

Jinx! I said that 2-1/2 years ago.

[lydia-duncan]

Michelle's prioritization spreadsheet says we've argued this is a bug fix, so removing the 2.0 label