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

[ refactor ] ScopedSnocList: WIP #3368

Draft
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

GulinSS
Copy link

@GulinSS GulinSS commented Aug 9, 2024

Description

We are going to take again a turn to solve TODO, related to the usage of SnocList at Scope. This change is heavily intrusive and its older attempt was already here.

By suggestion of @buzden we are going to start this heavy change from a very conceptually simple step: collect all usages and mark them. It would help us to schedule further changes at closed volume of overall affected codebase.

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG_NEXT.md (and potentially also
    CONTRIBUTORS.md).

@GulinSS GulinSS changed the title [ refactor] ScopedSnocList: WIP Draft: [ refactor] ScopedSnocList: WIP Aug 9, 2024
@GulinSS GulinSS changed the title Draft: [ refactor] ScopedSnocList: WIP [ refactor] ScopedSnocList: WIP Aug 9, 2024
@GulinSS GulinSS marked this pull request as draft August 9, 2024 13:21
@GulinSS GulinSS changed the title [ refactor] ScopedSnocList: WIP [ refactor ] ScopedSnocList: WIP Aug 9, 2024
Copy link
Member

@gallais gallais left a comment

Choose a reason for hiding this comment

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

I haven't looked at the whole PR but I have tagged a couple of issues.
This is why I was concerned: there's a lot of domain-specific knowledge
in the choice to represent something as a list vs. a snoclist.

src/Compiler/ANF.idr Outdated Show resolved Hide resolved
||| Constructor for a data type; bind the arguments and subterms.
ConCase : Name -> (tag : Int) -> (args : List Name) ->
CaseTree (args ++ vars) -> CaseAlt vars
ConCase : Name -> (tag : Int) -> (args : ScopedList Name) ->
Copy link
Member

Choose a reason for hiding this comment

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

It's debatable whether the constructor arguments should be listed right-to-left.
Also they would become the most local variables so the nested case tree would
have a type of the form CaseTree (vars <>< args) rather than args flowing into
the global scope.

Copy link
Author

Choose a reason for hiding this comment

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

Left for now as marked as you said it is a debatable question.

insertCaseAltNames p q (ConCase x tag args ct)
= ConCase x tag args
(rewrite appendAssociative args outer (ns ++ inner) in
(rewrite appendAssociative args outer (ns +%+ inner) in
Copy link
Member

Choose a reason for hiding this comment

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

appendAssociative is still List-specific

Copy link
Author

@GulinSS GulinSS Aug 10, 2024

Choose a reason for hiding this comment

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

Yep, current step is only renaming to mark existing usages of List in context of Scope. No structural changes for current step. Or I didn't get you correctly?

@GulinSS GulinSS force-pushed the scoped_snoc_list branch 5 times, most recently from c486736 to 14b674a Compare August 11, 2024 15:22
@GulinSS
Copy link
Author

GulinSS commented Aug 11, 2024

Took into consideration @gallais points where I did replacements where it should not be. Also asked @buzden how to determine where such replacements should be and should not be. He suggested to everytime take a look at https://github.com/edwinb/Yaffle/ for reference when I am getting stuck with questions does it should be Snoc or not. Reviewed my progress and changed code according to Yaffle.

@gallais,
@buzden,
much thanks!

I continue my work.

@GulinSS
Copy link
Author

GulinSS commented Aug 12, 2024

Well, I am in progress

229/286: Building Core.Case.CaseBuilder (src/Core/Case/CaseBuilder.idr)

@GulinSS GulinSS force-pushed the scoped_snoc_list branch 2 times, most recently from 9b9e8d5 to afd19ee Compare August 13, 2024 10:17
@GulinSS
Copy link
Author

GulinSS commented Aug 13, 2024

It compiles!

The next steps are to

  1. group all changes by their semantics
  2. groke translate rules from cons to snoc style
  3. apply them.

Starting work on the grouping.

@GulinSS
Copy link
Author

GulinSS commented Aug 15, 2024

Stuck tests running. The difference between the commit and main:

% diff tests/build/exec/runtests_app/runtests.ss tests/build/exec/runtests_app/runtests.ss.back
3c3
< ;; @generated by Idris 0.7.0-7098a379f, Chez backend
---
> ;; @generated by Idris 0.7.0-2482ebb43, Chez backend
654,655c654,655
< (define PrimIO-unsafeCreateWorld (lambda (arg-1) (arg-1 )))
< (define PrimIO-unsafePerformIO (lambda (arg-1) (PrimIO-unsafeCreateWorld (lambda (u--w) (let ((eff-0 (arg-1 ))) eff-0)))))
---
> (define PrimIO-unsafeCreateWorld (lambda (arg-1) (arg-1 #f)))
> (define PrimIO-unsafePerformIO (lambda (arg-1) (PrimIO-unsafeCreateWorld (lambda (u--w) (let ((eff-0 (arg-1 u--w))) eff-0)))))
688c688
<   
---
> 

For some reason (arg-1 <X>) writes into (arg-1 ) instead of (arg-1 #f) and (arg-1 u--w). #f and u--w are missed. 🤔

@GulinSS
Copy link
Author

GulinSS commented Aug 15, 2024

Added special inverted operators for current ScopedList. They purpose will be simulate usage of SnocList.

> (1 :%: 2 :%: Lin) +%+ (3 :%: 4 :%: Lin)
1 :%: (2 :%: (3 :%: (4 :%: [<])))
> (Core.Name.ScopedList.Lin :<%: 1 :<%: 2) +<%+ (Core.Name.ScopedList.Lin :<%: 3 :<%: 4)
4 :%: (3 :%: (2 :%: (1 :%: [<])))

ScopedSnocList: WIP: Remove logical dupe `Reflect (SnocList a)`
…ble`, `embed(N|Is)Var`, `locate(|N|Is)Var`, `weaken(|N)Var`, `insert(|N)Var`, `insertNVarNames`, `removeNVar`, `strengthen(*|N*|Is*)Var`
…ble`, `insertNVarNames` and `genWeaken` (fixed a mistake). Also snoc applied to `CompileExpr`, `Env`, `TT`, `CaseTree`, `Subst`
@GulinSS GulinSS force-pushed the scoped_snoc_list branch 2 times, most recently from 5a9881e to a0e5246 Compare October 4, 2024 09:49
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 this pull request may close these issues.

3 participants