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

Recognize if-like Tree.MultiSegmentApp as IfThenElse IR #11365

Draft
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Oct 19, 2024

Pull Request Description

Fixes #9165 by recognizing if/then/else in a special way. Introduces new IfThenElse IR element to represent both if_then_else as well as if_then cases. Such element needs to be recognized by various IR processing passes and at the end converted into executable IfThenElseNode in IrToTruffle.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach self-assigned this Oct 19, 2024
@JaroslavTulach JaroslavTulach marked this pull request as draft October 19, 2024 17:01
@JaroslavTulach JaroslavTulach requested a review from kazcw October 19, 2024 17:01
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 20, 2024

To keep compatibility with original if_then_else and if_then method invocations, following code:

from Standard.Base import all

type X
    Ok
    Bad

    if_then_else self ~t ~f = case self of
        X.Ok -> t
        X.Bad -> f

    if_then self ~t = case self of
        X.Ok -> t
        X.Bad -> Nothing

main =
    a = X.Ok.if_then_else "true" "false"
    b = X.Bad.if_then_else "true" "false"
    c = if X.Ok then "true" else "false"
    d = if X.Bad then "true" else "false"

    x = X.Ok.if_then "true"
    y = X.Bad.if_then "true"
    z = if X.Ok then "true"
    w = if X.Bad then "true"


    [a, b, c, d, x, y, z, w]

should print ['true', 'false', 'true', 'false', 'true', Nothing, 'true', Nothing]. As of fba42c5 it doesn't do that. E.g. compatibility is broken.

It has to be Boolean!

Decision: The condition of if then else controlflow construct has to yield a Boolean-like value. No dispatch to if_then_else method will happen. Use v.if_then_else (trueAction) (falseAction) if you insist on the method dispatch semantics.

@JaroslavTulach
Copy link
Member Author

Instrumenter_Spec tests are failing probably because we don't support instrumentation inside of case of statements!? fyi @4e6.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 20, 2024

While working on this PR I realized that we represent every case_of branch as a separate function. Can we speed something up by delaying the IR processing when we need such a closure?

Creating wip/jtulach/LazyIfCaseOfBranches9165 branch and measuring startup again and engine. Let's see the result!

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 21, 2024

After running engine benchmarks it looks like just converting IfThenElse to Case.Expr isn't really speeding up anything.

IfBench3 is slower

Quite contrary. The most essential benchmark is slowing down. I guess direct conversion of IfThenElse to a Truffle node is needed.

This state of work is now preserved as ConvertingIfThenElseToCaseExpr and this PR will try to go different direction.

mergify bot pushed a commit that referenced this pull request Oct 24, 2024
Related to #9165 work. Let's make sure the current behavior of `if`/`then`/`else` is properly tested by a unit test. Extracting test created as part of #11365 to verify it really describes the current behavior.
mergify bot pushed a commit that referenced this pull request Oct 29, 2024
Work on #11365 made me realize that `GraphOccurrence.Global` isn't really used anywhere. Simplifying the code by removing it and associated methods.
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 30, 2024

Standard library tests are green: https://github.com/enso-org/enso/actions/runs/11582091869/job/32244310882?pr=11365

Next: Fix error reporting - e.g. unit tests

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

The overall direction seems good. What are your intentions regarding scoping rules of the if-then-else branches?

boolean cond,
ExpressionNode trueBranch,
ExpressionNode falseBranch,
@Shared("profile") @Cached InlinedCountingConditionProfile profile) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: You don't have to specify the name "profile" here. It is OK to just specify @Shared. As long as the parameter name is the same as in the other specialization, the shared annotation will work as expected.


/** The Enso case expression. */
case class IfThenElse(
val cond: Expression,
Copy link
Member

Choose a reason for hiding this comment

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

nit: val modifier is redundant for parameter of case class primary constructor

}

@Test
public void variableNotVisibleAfterBranches() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Testing scopes of if then else is a good idea. I would like to see more expanded tests. Something like:

x = True
if x then
  y = True
  if x then if y then "Foo"

and

x = False
cond = False
if cond then "cond" else
  y = False
  if x then "x" else
    if y then "y" else 42

that is, try to use bindings from various parent scopes in the condition, true branch and false branch.

These tests could be placed in Base_Tests as well I believe.

.asInstanceOf[Application.Prefix]
.arguments(2)
.value
.asInstanceOf[IfThenElse]
Copy link
Member

Choose a reason for hiding this comment

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

IfThenElse as a builtin IR is indeed better than representing it as Application.Prefix with 2 arguments

t1 = table_builder [["X", [My_Type.Value 1 2, 2.0, 2]], ["Y", [10, 20, 30]]]
t2 = table_builder [["Z", [2.0, 1.5, 2.0]], ["W", [1, 2, 3]]]
r3 = t1.join t2 join_kind=Join_Kind.Inner on=(Join_Condition.Equals "X" "Z") on_problems=..Report_Warning
t8 = table_builder [["X", [My_Type.Value 1 2, 2.0, 2]], ["Y", [10, 20, 30]]]
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the requirement that you cannot override a binding from parent scope in the branches. The true branch and else branch is nothing more than a block of expressions, right? The following code:

foo ~action =
    action

main =
    x = 42
    foo <|
        x = 23
        x

prints 23, so it let's me override the x from the parent scope as well.

Is this the desired state or is this still WIP?

mergify bot pushed a commit that referenced this pull request Nov 1, 2024
Another change motivated by work on #11365. Continuation of #11419.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3x slower IfVsCaseBenchmarks_ifBench6In
3 participants