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

effects: replaces usages of ALWAYS_FALSE with TRISTATE_UNKNOWN #44808

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

aviatesk
Copy link
Sponsor Member

Although they are basically equivalent in the current implementation,
it would be more correct conceptually if we propagate TRISTATE_UNKNOWN
instead of ALWAYS_TRUE, as it is hard or impossible to derive a global
conclusion from a local analysis on each statement in most places.

This commit also adds a docstring that simply explains the design of the
effect analysis.

Although they are basically equivalent in the current implementation,
it would be more correct conceptually if we propagate `TRISTATE_UNKNOWN`
instead of `ALWAYS_TRUE`, as it is hard or impossible to derive a global
conclusion from a local analysis on each statement in most places.

This commit also adds a docstring that simply explains the design of the
effect analysis.
@aviatesk aviatesk requested a review from Keno March 31, 2022 11:44
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

I'm ok with this, though we may have to undo it and actually make use of this properly in a few weeks, but until then this is probably cleaner.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 31, 2022
@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Apr 1, 2022

actually make use of this properly in a few weeks

Well, how do we really want to use it? It sounds a bit tricky to make a global conclusion from local analysis. Maybe use a sort of simple domination analysis like linear_inline_eligible?:

function linear_inline_eligible(ir::IRCode)
length(ir.cfg.blocks) == 1 || return false
terminator = ir[SSAValue(last(ir.cfg.blocks[1].stmts))][:inst]
isa(terminator, ReturnNode) || return false
isdefined(terminator, :val) || return false
return true
end

@aviatesk aviatesk merged commit cf649a7 into master Apr 1, 2022
@aviatesk aviatesk deleted the avi/always_unknown branch April 1, 2022 04:54
aviatesk added a commit that referenced this pull request Apr 1, 2022
…44808)

Although they are basically equivalent in the current implementation,
it would be more correct conceptually if we propagate `TRISTATE_UNKNOWN`
instead of `ALWAYS_TRUE`, as it is hard or impossible to derive a global
conclusion from a local analysis on each statement in most places.

This commit also adds a docstring that simply explains the design of the
effect analysis.
@aviatesk aviatesk mentioned this pull request Apr 1, 2022
67 tasks
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 1, 2022
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.

4 participants