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

Add documentation to the main 'Interflow' class and refactor some APIs #3619

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

Conversation

kyouko-taiga
Copy link
Contributor

This PR is simply adding documentation and renaming parts of the internal API for clarity.

Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

Looks good so far, I've answered to the questions left in comments

private val modulePurity = mutable.Map.empty[nir.Global.Top, Boolean]

// QUESTION: What does "Tl" mean?
Copy link
Contributor

Choose a reason for hiding this comment

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

In this context it stand for ThreadLocal. All of these methods are simple accessor for thread local value of given type, eg. A Thread Local nir.Fresh used to create new SSA identifiers. ThreadLocal values can be used when releaseMode=Debug as it allows for parallel optimisation of NIR by applying only a subset of optimisations. In other modes (ReleaseX) there's currently only a 1 thread performing optimization pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it relevant to keep the Tl suffix in these methods then? It seems to me like it's simply a property that should be documented in the methods. Besides, we don't have any non thread local counterparts to these methods, suggesting that the qualification is perhaps superfluous.

private val modulePurity = mutable.Map.empty[nir.Global.Top, Boolean]

/** Returns the current scope. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of a performative comment. It would be nice if we could describe what a scope is more precisely.

Copy link
Contributor

Choose a reason for hiding this comment

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

ScopedVar is a list a stack-like variable which can be appended when entering some scope and removed from the stack when existing the scope. Eg.

scoped(currentDefn := Foo):
  currentDefn.get // yields Foo
  scoped(currentDefn := Bar): 
    currentDefn.get // yields Bar
  currentDefn.get // yield.Foo  

These are uses to preserve current state when entering some nested area of code without usage of global variables and manually reasignnning them with old value.

In case of currentFreshScope thats a ThreadLocal[ScopedVar[nir.Fresh]] so each thread has it's own stack of nir.Fresh generators. When it starts to inline a method it would create a new nir.Fresh on top of the stack and would use it in the MergeProcessor. When it's done it would pop this value.

/** In debug mode, visits all reachable definitions from defined entries. In
* release mode, visits all entry symbols.
*/
// QUESTION: What is exactly an entry?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WojciechMazur I need your lights here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Entry is a root symbol from which we should start visting the methods.
Typically it would be the main method symbol.
In case of using ScalaNative to create a library entry would be every method annotated with @exported found on the classpath.

There also exist a synthetic entries - for example a list of symbols that might not be directly reachable from the organic roots, but might be required in optimizer or in lowering stage (see Lower.depends, Generate.depends, Interflow.depends)

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.

None yet

2 participants