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 52 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.
@@ -3441,6 +3448,7 @@ lazy val `runtime-compiler-dump-igv` =
(project in file("engine/runtime-compiler-dump-igv"))
.enablePlugins(JPMSPlugin)
.settings(
customFrgaalJavaCompilerSettings("21", true),
Copy link
Member Author

Choose a reason for hiding this comment

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

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 3, 2025

With the help of #12061 I can see the IfThenElse IR is shared from two different methods (normal one and its static copy presumably):

shared IfThenElse

Which brings us a fix: ad7b2a7

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 3, 2025

Looks like our Base_Tests are fine now:

sbt:enso> runEngineDistribution --run test/Base_Tests
2944 tests succeeded.
0 tests failed.
48 tests skipped.
17 groups skipped.

running unit tests...

sbt:enso> runtime-integration-tests/test
[info] *** 16 TESTS FAILED ***
[error] Failed: Total 26129, Failed 18, Errors 0, Passed 26111, Ignored 23, Pending 30
[error] Failed tests:
[error]         org.enso.interpreter.test.semantic.LambdaTest
[error]         org.enso.interpreter.test.semantic.CompileDiagnosticsTest
[error]         org.enso.interpreter.test.instrument.RuntimeServerTest
[error]         org.enso.interpreter.test.semantic.MixfixFunctionsTest
[error]         org.enso.compiler.test.IfThenElseTest
[error]         org.enso.interpreter.test.semantic.StrictCompileDiagnosticsTest
[error]         org.enso.interpreter.test.semantic.LambdaShorthandArgsTest
[error]         org.enso.compiler.pass.analyse.test.AliasAnalysisTest
[error]         org.enso.interpreter.test.instrument.RuntimeComponentsTest
[error]         org.enso.interpreter.test.instrument.RuntimeStdlibTest

That's not that bad - after months of not being able to make a progress, that's actually not that far from ideal.

Copy link
Member

Choose a reason for hiding this comment

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

Since #11770 is merged, try to implement IfThenElse like CallArgument and not as Scala case class.

Copy link
Member Author

@JaroslavTulach JaroslavTulach Feb 4, 2025

Choose a reason for hiding this comment

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

That's indeed desirable. Done in 40edc67 and it looks great, simple and easy to use. Except few gotchas in the generated code:

  • protected diagnostics, passData, location are wrong - they shouldn't be protected, but private
  • Objects.equals(this.passData, other.passData) is wrong (and I am surprised it even works) - it should be this.passData == other.passData
  • showCode(int) should be abstract to force implementors to implement it

@enso-bot enso-bot bot mentioned this pull request Feb 4, 2025
2 tasks
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.

IfThenElse IR to avoid 3x slower IfVsCaseBenchmarks_ifBench6In
3 participants