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

update context-independent and remove context-dependent lowering of ' #25148

Merged
merged 5 commits into from
Dec 19, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 17, 2017

This pull request is the next step towards JuliaLang/LinearAlgebra.jl#57 after #24969, #25083, and #25125, completing items 13-17 in #24969's OP's task list. Specifically, this pull request:

  1. Rewrites all semantically eager 's in base, test, and stdlib as appropriate.
  2. Changes lowering of X' to postfixapostrophize(X), and provides fallback = Adjoint(X).
  3. Removes the special lowering for ' in multiplication, left-division, and right-division expressions.

Notes:

  1. This pull request is based on remove special lowering for and deprecate .' #25125, from which the first five commits come. Only the last five commits belong to this pull request. I will rebase those commits out once remove special lowering for and deprecate .' #25125 merges. Rebased out.
  2. I plan to write a news entry for these changes as part of a large, single news item on the A_mul_B and all that jazz LinearAlgebra.jl#57 (in a later pull request).

Thanks and best!

@Sacha0 Sacha0 added linear algebra Linear algebra compiler:lowering Syntax lowering (compiler front end, 2nd stage) needs news A NEWS entry is required for this change labels Dec 17, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 18, 2017

Rebased and fixed the few test failures CI revealed. Assuming CI approves and absent objections or requests for time, I plan to merge these changes this evening PT or later and dive into the last stretch of JuliaLang/LinearAlgebra.jl#57. Best!

base/exports.jl Outdated
@@ -248,6 +248,7 @@ export
At_mul_Bt!,
At_rdiv_B,
At_rdiv_Bt,
postfixapostrophize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to be exported?

Copy link
Member Author

@Sacha0 Sacha0 Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems so? Otherwise one must import it broadly?

@TotalVerb
Copy link
Contributor

Why not lower x' to Expr(:call, Symbol("'"), :x)?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 18, 2017

Why not lower x' to Expr(:call, Symbol("'"), :x)?

Path of least resistance and uncertainty mostly :).

@TotalVerb
Copy link
Contributor

Can also lower to (call (core postfixapostrophe) ,x) like the lowering of a.x to avoid needing to export.

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 19, 2017

Can also lower to (call (core postfixapostrophe) ,x) like the lowering of a.x to avoid needing to export.

Yes, @fredrikekre and I just came to the same conclusion on slack. That's much better. Will fix and push. Thanks! :)

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 19, 2017

x' now lowers to the not-exported Core.postfixapostrophize(x). Thanks!

@StefanKarpinski
Copy link
Member

Are we going to have to change the syntax for all of these or are these just temporary?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 19, 2017

Are we going to have to change the syntax for all of these or are these just temporary?

Sorry, for all of what?

@StefanKarpinski
Copy link
Member

A' to adjoint(A), etc.

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 19, 2017

The A' -> adjoint(A) transformations in this pull request guard against the change in lowering causing behavioral changes: The change in lowering impacts neither the behavior of A's appearing in mul, ldiv, and rdiv expressions, nor the behavior of A's generally where A::AbstractVector. But A's appearing in isolation (i.e. not in a mul, ldiv, or rdiv expression) where A::AbstractMatrix are presently eager (adjoint(A)), so the change in lowering does impact their behavior (->Adjoint(A)). Though in most cases Adjoint(A) will serve as well or better going forward, the defensive A' -> adjoint(A) transformations minimize unexpected behavioral changes in the transition. Does that answer your question? :)

@Sacha0 Sacha0 merged commit 1319567 into JuliaLang:master Dec 19, 2017
@Sacha0 Sacha0 deleted the despecial branch December 19, 2017 22:20
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 19, 2017

Thanks all!

@ararslan
Copy link
Member

This introduced a new warning during the build:

WARNING: Method definition postfixapostrophize(Any) in module Inference at operators.jl:743 overwritten in module Base at operators.jl:743.

@@ -2396,7 +2367,7 @@
,.(apply append rows)))
`(call (top typed_vcat) ,t ,@a)))))

'|'| (lambda (e) (expand-forms `(call adjoint ,(cadr e))))
'|'| (lambda (e) (expand-forms `(call (core postfixapostrophize) ,(cadr e))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use top not core, and put separate definitions inside each topmodule (Inference and Base) instead of in boot.jl. That'll fix the method overwrite issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vtjnash! Seems Jeff is addressing the warning in #25212 :).

@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants