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

Promoted addition on arrays with mismatched shape #15914

Closed
rahulghangas opened this issue Jun 23, 2020 · 8 comments · Fixed by #26608
Closed

Promoted addition on arrays with mismatched shape #15914

rahulghangas opened this issue Jun 23, 2020 · 8 comments · Fixed by #26608

Comments

@rahulghangas
Copy link
Member

rahulghangas commented Jun 23, 2020

Reproducible on 1.22.0

var A : [1..5, 1..5] int = 0;
var B : [1..3, 1..3] int = 1;

A += B;

writeln(A);

The part of A not corresponding to B gets filled with random values.

An example output is

1 1 1 1 1
1 1 1 1 1
1 1 1 0 62
0 62 1 4370556800 4370534808
4370556800 4370534808 0 0 0

As a user, I would expect
A.) the promoted operator to be applied only to the part of A corresponding to B, OR
B.) Throw an error for mismatched arrays

@rahulghangas
Copy link
Member Author

@bradcray tagging you since you've been associated with similar issues.

@rahulghangas rahulghangas changed the title Promoted addition on mismatched arrays Promoted addition on arrays with mismatched shape Jun 23, 2020
@bradcray
Copy link
Member

Hi @rahulghangas — I believe that this is a variation on #14264 and #11428, which are challenging issues to solve. However, because this example is specific to a promoted operator like +=, we could potentially handle it more easily by providing array-specific overloads of the op= operators that do a size check before computing the expression. I'm pretty sure that, at present, such calls are resolving to the generic scalar += overload and promoting it, such that the compiler is essentially rewriting such an expression as forall (a,b) in zip(A,B) do a += b; and falling into the identical problem of those other issues. Imagine instead we provided something like:

proc +=(X: [], Y: []) {
  if (X.shape != Y.shape) then
    halt("Shape mismatch in applying += to arrays: ", X.shape, " != ", Y.shape);
  forall (x,y) in zip(X,Y) do
    x += y;
}

Here's your example extended to demonstrate this: TIO

@rahulghangas
Copy link
Member Author

Gotcha, thanks for pointing out the related issues. I think this can be closed then?

@e-kayrakli
Copy link
Contributor

I'll just add that an array-specific overload for such operators can have the additional benefit of doing some optimizations especially for distributed arrays.

Of course it'd be great if the compiler can do such optimizations on that forall loop, but doing that in the operator overload is much more tractable, and even in such a world, there might be things we can do better in the operator overload.

As for closing, this issue may not be a completely redundant copy of the existing ones, so maybe it should stay open? No strong preference from me, though

@bradcray
Copy link
Member

I don't think I'd close it, at least for the op= case, since that seems tractable for us to address in the current code organization and module system. Even though it's not a complete solution, I think it'd be attractive to provide these overloads and protect the users in the cases we can. I might rewrite the title to focus on op=, though, for that reason rather than general promotion (which I agree is more like a duplicate of the existing issues).

can have the additional benefit of doing some optimizations especially for distributed arrays.

This isn't clear to me... what are you seeing / anticipating?

@e-kayrakli
Copy link
Contributor

This isn't clear to me... what are you seeing / anticipating?

Even if the compiler can do communication aggregation in some fashion, there might be other things we can do that depend on the type of the operands. If they are irregular arrays for example, we can think of ways to improve the operation in ways that the compiler can't (at least not easily). An example that I can think of is adding two sparse vectors. IRVs of the vectors can play a role in how that operation is done in an optimized fashion for example.

Even without such a case, it philosophically made sense to me to have both ways of optimizing such things. I can imagine having an optional doiPlusEquals that does something more esoteric as I described above, and if it is not implemented we fallback to forall which may or may not be optimized by the compiler.

@bradcray

This comment has been minimized.

@e-kayrakli

This comment has been minimized.

bradcray added a commit to bradcray/chapel that referenced this issue Jan 27, 2025
I believe that chapel-lang#17947
resolved this.

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
bradcray added a commit that referenced this issue Jan 27, 2025
[trivial, not reviewed]

I believe that PR #17947 is
the one that resolved this test case.

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

Successfully merging a pull request may close this issue.

3 participants