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

Should A .= B evaluate to the RHS of the .=? #25954

Closed
mbauman opened this issue Feb 8, 2018 · 26 comments
Closed

Should A .= B evaluate to the RHS of the .=? #25954

mbauman opened this issue Feb 8, 2018 · 26 comments
Assignees
Labels
broadcast Applying a function over a collection compiler:lowering Syntax lowering (compiler front end, 2nd stage)

Comments

@mbauman
Copy link
Member

mbauman commented Feb 8, 2018

Currently, A .= B lowers to effectively broadcast!(identity, A, B). That typically returns the mutated object — A. But this isn't enforced… or even documented.

If we want to support chained .= expressions, it seems like we really need this to always evaluate to the RHS through a lowering transformation.

@mbauman mbauman added compiler:lowering Syntax lowering (compiler front end, 2nd stage) triage This should be discussed on a triage call broadcast labels Feb 8, 2018
@chethega
Copy link
Contributor

chethega commented Feb 8, 2018

So you want to fuse the loops in A .= (B.= C .+ D) .* E, in order to get multiple-output broadcasts? Or am I understanding you wrong here?

What do you do if an L-value appears multiple time, e.g. A .= (A .= A.+C) .+ A? Do you want, post this expression, A= 2*A+2*C or A = 2*A + 3*C? The first one, I presume.

@Keno
Copy link
Member

Keno commented Feb 8, 2018

It should be consistent with regular assignment, i.e. lower to the RHS.

@Keno
Copy link
Member

Keno commented Feb 8, 2018

Note that doing this as a lowering transform is not without precedent. setindex! returns the mutated object, but a[] = b returns b.

@mbauman
Copy link
Member Author

mbauman commented Feb 8, 2018

Yup, it's not so much about supporting fusion through multiple .= expressions as it is getting the semantics consistent.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Feb 8, 2018
@mbauman
Copy link
Member Author

mbauman commented Feb 8, 2018

Triage was unanimous. Let's do this.

@JeffBezanson
Copy link
Member

Triage accepts.

@mbauman mbauman removed the triage This should be discussed on a triage call label Feb 8, 2018
@stevengj
Copy link
Member

stevengj commented Feb 8, 2018

I'm not sure what the proposed semantics are.

Suppose you do

result = (floatarray .= 2 .* intarray)

What is "the RHS" in this case? The RHS expression2 .* intarray by itself would create another integer array, but because of fusion this array is never actually allocated here. The only place the result is stored is in the LHS floatarray, at which point the element type has changed and some rounding may have occurred.

Returning the LHS, on the other hand, would be straightforward. At minimum, we could document this property of broadcast!, but of course that couldn't be enforced. So we could lower to an expression let tmp = floatarray; broadcast!(...); tmp; end.

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2018

Is there a way in which we can expand x .= f.(y) as itself just a call to (lazy) broadcast (roughly along the lines of lazy(.=, x, lazy(f, y)))?

@mbauman
Copy link
Member Author

mbauman commented Feb 8, 2018

Very good point. Apparently we have the technology in lowering to wrap expressions in "unnecessary" heads, and then they can get elided if they're never used. That's still a little unsatisfactory, since it'd mean that we'd run the broadcasted function(s) through the arguments twice — once in the fused assignment and once in allocation of the RHS.

The goal here would be to try to lower to something that would detect if it's needed.

@stevengj
Copy link
Member

stevengj commented Feb 8, 2018

Would it be so terrible to just have .= return the LHS, not the RHS, since the LHS is always materialized? It would be different from =, but .= is already different from = in lots of ways.

@Keno
Copy link
Member

Keno commented Feb 8, 2018

Would it be so terrible to just have .= return the LHS

Yes, it's inconsistent with every other appearance of = in the language.

@JeffBezanson
Copy link
Member

The lowering passes (julia-syntax.scm) have a feature where you can wrap an expression in (unnecessary ...) and it will be removed if the value is not used.

@stevengj
Copy link
Member

stevengj commented Feb 9, 2018

.= is already very different from = ... if you are mutating an object in-place, doesn’t it make sense to return the mutated object?

@JeffBezanson
Copy link
Member

a[i] = b is also a mutation though.

@raminammour
Copy link
Contributor

I am not a computer scientist, so excuse my ignorance. What happens to codes that follow this pattern:

struct MyType{T,N}<: AbstractArray {T,N}
    A::Array{T,N}
    Other fields...
end

Base.setindex!(m::MyType,I,v)=setindex!(m.A,I,v)....

f(m::MyType)=...

m=MyType(...)

m.=rand(10)

f(m)

Will m become an array and the function call not work anymore? Is this pattern not common enough, or wrong?

Thanks

@mbauman
Copy link
Member Author

mbauman commented Feb 9, 2018

No, this will be nearly unobservable in most everyone's code. This is only considering what the entire expression (m.=rand(10)) should evaluate to. In most code, you don't even ask for this value. You'll only observe this change if your code asks for ret = (m.=rand(10)) or ret .= (m.=rand(10))… and it'll determine what gets returned at the REPL, too.

The .= syntax will still continue assigning into and mutating objects as you expect.

@tkf
Copy link
Member

tkf commented Feb 11, 2018

I've been using something like A_mul_B!(y, A, (x .= x.^2)) to avoid allocation. Does it mean I need to rewrite it in two lines in Julia 1.0? (I do respect consistency so the rewrite is fine, but it was a very handy notation...)

@chethega
Copy link
Contributor

Could we see some more examples where this change matters?

Stevegj's example:
result = (floatarray .= 2 .* intarray)
LHS lowering: one loop, no temp copy, result===floatarray

RHS lowering: What should this mean? What code should get run? Do we store a temporary or compute twice?

result .= (floatarray .= 2 .* intarray)

LHS lowering: two loops, perform compute once, no temp copy, result == floatarray (mod inexact conversions), result !== floatarray.

RHS lowering: What should this mean? What code should get run? Do we run all computations in the (ultimately) RHS multiple times? Do we store them in a temporary?

Is there any example where trying for RHS lowering is better than returning nothing? What are the examples where returning the LHS is better than returning nothing?

Long-term, fusing into multi-output broadcasts might be cool; in this case we'd best have something now that returns the same result as the eventual solution (since multi-output broadcast that can store temporaries on the way has no syntax now).

To give an example where we currently have a difference:

a=100;c=1;a= (a + 0) + (a=a+c); a
#201
a=[100];c=[1]; a= (a + 0) + (a=a+c); a
#[201]
a=[100];c=[1]; @. a= (a + 0) + (a=a+c); a
#[202]

Otoh

a=100;c=1;a= a  + (a=a+c); a
#202
a=[100];c=[1]; a= a  + (a=a+c); a
#[202]
a=[100];c=[1]; @. a= a  + (a=a+c); a
#[202]

@mbauman
Copy link
Member Author

mbauman commented Mar 1, 2018

I'm getting false positives in #24368 with code like this:

$ ./julia --depwarn=error -q
julia> module TestFoo
           x .= y
           nothing
       end
ERROR: syntax: Deprecated syntax `using the value of `.=``.

Edit: whoops, I meant to post this in the PR that fixed this issue.

mbauman added a commit that referenced this issue Mar 5, 2018
…gnment

* master:
  Make stdlib tests runnable standalone. (#26242)
  fix unary-related parsing regressions caused by #26154 (#26239)
  Formatting changes to new SSA IR devdocs [ci skip]
  use medium code model on PPC
  `retry` should support the check function proposed in the docstring. (#26138)
  mention axes in docs instead of size (#26233)
  exclude more CI files from source distro (#25906)
  Describe three-valued logic in docstrings
  deprecate using the value of `.=`. fixes #25954 (#26088)
  backport change to make CodegenPrepare work with isNoopAddrSpaceCast
  optimize the python version of abs2 in the microbenchmarks (#26223)
  Add notes for package maintainers (#25912)
  typo
  Fix broken links to functions in the manual (#26171)
  [NewOptimizer] Track inlining info
  Begin work on new optimizer framework
  add patch to make GC address spaces work on PPC
  also backport sover patch to LLVM 4.0
@stevengj
Copy link
Member

stevengj commented Mar 7, 2018

I keep encountering cases where this change makes it impossible to chain in-place operations. e.g. I just ran into a case where I wanted shuffle!(a .= foo.(b)), and instead you have to split it into two statements. I really think returning the LHS is the desirable behavior here.

@mbauman
Copy link
Member Author

mbauman commented Mar 7, 2018

Or you can use shuffle!(map!(foo, a, b)).

@stevengj
Copy link
Member

stevengj commented Mar 7, 2018

@mbauman, in this case I actually had foo.(bar.(b)), but yes, I could have used shuffle!(map!(x -> foo(bar(x)), a, b)). Or I could split it into two lines. I'm just saying that chaining a sequence of in-place operations is extremely common, and it is a shame to prohibit using .= in this context.

@StefanKarpinski
Copy link
Member

I have to say I'm fairly convinced by @stevengj's examples. Is there some way we can return the LHS and still avoid the spooky type annotation action at a distance?

@Keno
Copy link
Member

Keno commented Mar 7, 2018 via email

@stevengj
Copy link
Member

stevengj commented Mar 7, 2018

Returning the rhs in any form loses the advantage of working in-place with the lhs, which is the whole point of .=. I really don't see the problem here with .= being different from = in yet another way.

@stevengj
Copy link
Member

And now we can't use .= in the REPL without getting a deprecation warning...

JeffBezanson added a commit that referenced this issue Apr 9, 2018
JeffBezanson added a commit that referenced this issue Apr 9, 2018
JeffBezanson added a commit that referenced this issue Apr 12, 2018
mbauman added a commit that referenced this issue Apr 12, 2018
* origin/master:
  A few more #26670 fixes (#26773)
  Revert "deprecate using the value of `.=`. fixes #25954" (#26754)
  change dim arguments for `diff` and `unique` to keyword args (#26776)
  reorder pmap arguments to allow do-block syntax (#26783)
  correct deprecated parametric method syntax (#26789)
  [NewOptimizer] handle new IR nodes correctly in binary format
  [NewOptimizer] support line number emission from new IR format
  fix #26453, require obviously-concrete lower bound for a var to be diagonal (#26567)
  fix #26743, spurious `return` path in try-finally in tail position (#26753)
  Also lift SelectInst addrspaces
@mbauman mbauman added the broadcast Applying a function over a collection label Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

No branches or pull requests

9 participants