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

Use BuiltinRootNode.ArgNode to extract argument for a builtin method #12201

Merged
merged 32 commits into from
Feb 8, 2025
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
e261e8d
Use BuiltinRootNode.ArgNode to extract argument for a builtin method
JaroslavTulach Jan 30, 2025
5847b0f
Gathering info from all arguments via ArgContext
JaroslavTulach Jan 30, 2025
b646932
Compressing flags into a byte
JaroslavTulach Jan 31, 2025
20fa9c3
Only convert when is(REQUIRES_CAST)
JaroslavTulach Jan 31, 2025
731f3bb
Process warnings in ArgNode
JaroslavTulach Jan 31, 2025
ecb74c5
Generate ArgNode for each positional argument
JaroslavTulach Jan 31, 2025
0925fa2
Cleaning up to address IDE's lints
JaroslavTulach Jan 31, 2025
24fe8da
Allow null return value from processArgument
JaroslavTulach Jan 31, 2025
f60457c
Removing unused variable
JaroslavTulach Jan 31, 2025
feb75d0
Merge remote-tracking branch 'origin/develop' into wip/jtulach/Builti…
JaroslavTulach Feb 3, 2025
8f652cb
Removing commented out code
JaroslavTulach Feb 4, 2025
01be17a
Merge branch 'wip/jtulach/BuiltinArgNode11827' of enso:enso-org/enso …
JaroslavTulach Feb 4, 2025
f0480f7
Merge remote-tracking branch 'origin/develop' into wip/jtulach/Builti…
JaroslavTulach Feb 4, 2025
e2571a1
Insert the node when adding it as a @Child
JaroslavTulach Feb 4, 2025
91d3584
Only process WithWarnings to be compatible with Warnings_Spec
JaroslavTulach Feb 4, 2025
847bd99
Introducing generateArguments again
JaroslavTulach Feb 4, 2025
7be3262
Fixing RuntimeErrorsTest
JaroslavTulach Feb 4, 2025
d6d859a
Avoid cast when type is Object
JaroslavTulach Feb 4, 2025
4ca2226
Throw the sentinal to make RuntimeVisualizationsTest happier
JaroslavTulach Feb 4, 2025
13fc5fe
Only generate ArgNode for positional argument
JaroslavTulach Feb 4, 2025
3579139
Merge remote-tracking branch 'origin/develop' into wip/jtulach/Builti…
JaroslavTulach Feb 4, 2025
934deae
Accept simple benchmark-results.xml name
JaroslavTulach Feb 5, 2025
e3ee0ea
Split the benchmarks to multiple lines
JaroslavTulach Feb 5, 2025
f211c19
Shield against exceptional states
JaroslavTulach Feb 5, 2025
54751cb
Removing superfluous if
JaroslavTulach Feb 5, 2025
68a654b
Removing support for IS_ARRAY
JaroslavTulach Feb 5, 2025
7364a34
Merge remote-tracking branch 'origin/develop' into wip/jtulach/Builti…
JaroslavTulach Feb 6, 2025
0a3ec13
Don't use InteropLibrary for primitive Enso values comparision
JaroslavTulach Feb 7, 2025
21d9bc3
Use EnsoHashMap.generation when concatenating two maps together
JaroslavTulach Feb 7, 2025
e6af654
Let SliceArrayVectorNode handle warnings on its own
JaroslavTulach Feb 7, 2025
560e6b7
Order of some warnings got reversed
JaroslavTulach Feb 7, 2025
5443128
Avoid the need fore corepack when buildEngineDistribution
JaroslavTulach Feb 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ public class AtomBenchmarks {
import Standard.Base.Data.Numbers

main = length ->
generator = acc -> i -> if i == 0 then acc else @Tail_Call generator (List.Cons i acc) (i - 1)
generator = acc -> i ->
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
if i == 0 then acc else
list = List.Cons i acc
less = i - 1
@Tail_Call generator list less

res = generator List.Nil length
res
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ public void initializeBenchmark(BenchmarkParams params) throws Exception {
"""
from Standard.Base import all

all_length v = v.fold 0 (sum -> str -> sum + str.length)
all_length v = v.fold 0 sum-> str->
sum + str.length

create rep len =
s = "Long string".repeat rep
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package org.enso.interpreter.node.expression.builtin;

import com.oracle.truffle.api.interop.TruffleObject;
import org.enso.interpreter.dsl.BuiltinType;

@BuiltinType(name = "Standard.Base.Any.Any")
public class Any extends Builtin {
public final class Any extends Builtin {
public Any() {
super(TruffleObject.class);
}

@Override
public Class<? extends Builtin> getSuperType() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@
// Before moving this definition to the `bool` package, as we should, one would have to address that
// problem first.
@BuiltinType(name = "Standard.Base.Data.Boolean.Boolean")
public class Boolean extends Builtin {
public final class Boolean extends Builtin {
public Boolean() {
super(java.lang.Boolean.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we attempt to provide a representationType to each Builtin. That way we can deduce from signature methods having argument like boolean that the desired builtin is Boolean and the Enso type should be Standard.Base.Data.Boolean.Boolean. With that information we can extract (via CastToNode) such a type from EnsoMultiValue....

}

@Override
protected List<Cons> getDeclaredConstructors() {
return List.of(new Cons("False"), new Cons("True"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,33 @@ private AtomConstructor build(EnsoLanguage language, ModuleScope.Builder scope,
}
}

private final Class<?> representationType;
private final String name;

public Builtin() {
name = this.getClass().getSimpleName().replaceAll("([^_A-Z])([A-Z])", "$1_$2");
protected Builtin(String representationType) {
this(findType(representationType));
}

protected Builtin(Class<?> representationType) {
this.representationType = representationType;
this.name = this.getClass().getSimpleName().replaceAll("([^_A-Z])([A-Z])", "$1_$2");
}

private static Class<?> findType(String fqn) {
try {
return Class.forName(fqn);
} catch (ClassNotFoundException ex) {
throw new IllegalArgumentException(ex);
}
}

private @CompilerDirectives.CompilationFinal Type type;
private @CompilerDirectives.CompilationFinal(dimensions = 1) AtomConstructor[] constructors;

public final boolean isRepresentedBy(Class<?> clazz) {
return representationType == clazz;
}

protected Class<? extends Builtin> getSuperType() {
return Any.class;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,39 @@
package org.enso.interpreter.node.expression.builtin;

import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.dsl.Cached;
import com.oracle.truffle.api.dsl.Fallback;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.interop.TruffleObject;
import com.oracle.truffle.api.interop.UnsupportedMessageException;
import com.oracle.truffle.api.nodes.Node;
import com.oracle.truffle.api.nodes.NodeInfo;
import com.oracle.truffle.api.nodes.RootNode;
import com.oracle.truffle.api.profiles.BranchProfile;
import org.enso.interpreter.EnsoLanguage;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.data.EnsoMultiValue;
import org.enso.interpreter.runtime.data.Type;
import org.enso.interpreter.runtime.data.hash.EnsoHashMap;
import org.enso.interpreter.runtime.data.hash.HashMapInsertAllNode;
import org.enso.interpreter.runtime.error.DataflowError;
import org.enso.interpreter.runtime.error.PanicException;
import org.enso.interpreter.runtime.error.PanicSentinel;
import org.enso.interpreter.runtime.warning.AppendWarningNode;
import org.enso.interpreter.runtime.warning.WarningsLibrary;
import org.enso.pkg.QualifiedName;

/** Root node for use by all the builtin functions. */
@NodeInfo(shortName = "BuiltinRoot", description = "Root node for builtin functions.")
public abstract class BuiltinRootNode extends RootNode {
private QualifiedName moduleName;
private QualifiedName typeName;

protected BuiltinRootNode(EnsoLanguage language) {
super(language);
}

protected QualifiedName moduleName = null;
protected QualifiedName typeName = null;

/** Get the module name where the builtin is defined. */
public QualifiedName getModuleName() {
return moduleName;
Expand Down Expand Up @@ -52,4 +70,159 @@ public void setTypeName(QualifiedName typeName) {
*/
@Override
public abstract String getName();

protected static final class ArgContext {
private TruffleObject returnValue;
private EnsoHashMap warnings;

public ArgContext() {}

public TruffleObject getReturnValue() {
return returnValue;
}

public boolean hasWarnings() {
return this.warnings != null;
}

private void addWarnings(
VirtualFrame frame, HashMapInsertAllNode insertNode, EnsoHashMap newWarnings) {
if (this.warnings == null) {
this.warnings = newWarnings;
} else {
int maxWarnings = EnsoContext.get(insertNode).getWarningsLimit();
this.warnings = insertNode.executeInsertAll(frame, this.warnings, newWarnings, maxWarnings);
}
}
}

protected abstract static class ArgNode extends Node {
private static final byte IS_SELF = 0x01;
private static final byte REQUIRES_CAST = 0x04;
private static final byte CHECK_ERRORS = 0x08;
private static final byte CHECK_PANIC_SENTINEL = 0x10;
private static final byte CHECK_WARNINGS = 0x20;
private final byte flags;
@CompilerDirectives.CompilationFinal private Type ensoType;

@Child private WarningsLibrary warnings;
@Child private AppendWarningNode appendWarningNode;
@Child private HashMapInsertAllNode mapInsertAllNode;

private final BranchProfile errorsTaken = BranchProfile.create();
private final BranchProfile sentinelTaken = BranchProfile.create();

ArgNode(byte flags) {
this.flags = flags;
if (is(CHECK_WARNINGS)) {
this.warnings = WarningsLibrary.getFactory().createDispatched(5);
}
}

private boolean is(byte what) {
return (flags & what) != 0;
}

@SuppressWarnings("unchecked")
public final <T> T processArgument(
VirtualFrame frame, Class<T> type, Object value, ArgContext context) {
assert value != null;
if (is(CHECK_ERRORS) && value instanceof DataflowError err) {
errorsTaken.enter();
context.returnValue = err;
return null;
}
if (is(CHECK_PANIC_SENTINEL) && value instanceof PanicSentinel sentinel) {
sentinelTaken.enter();
throw sentinel;
}
if (warnings != null) {
if (warnings.hasWarnings(value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Using hasWarnings here instead of simple warnings instanceof WithWarnings is causing problems:

Let's see what #12258 can do.

if (mapInsertAllNode == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
this.mapInsertAllNode = insert(HashMapInsertAllNode.build());
}
try {
context.addWarnings(frame, mapInsertAllNode, warnings.getWarnings(value, false));
value = warnings.removeWarnings(value);
} catch (UnsupportedMessageException ex) {
throw raise(RuntimeException.class, ex);
}
}
}
if (is(REQUIRES_CAST) && type != Object.class) {
var ctx = EnsoContext.get(this);
if (this.ensoType == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The first time ArgNode is requested to "cast", it searches for the ensoType representing the value of the argument. It would be better to do this when the ArgNode is being created, but it may be too early (before Standard.Base types are loaded in), so I haven't even tried.

Opinions?

CompilerDirectives.transferToInterpreterAndInvalidate();
var builtin = ctx.getBuiltins().getByRepresentationType(type);
if (builtin == null) {
this.ensoType = ctx.getBuiltins().any();
} else {
this.ensoType = builtin.getType();
}
}
var conv = executeConversion(value);
if (conv == null) {
CompilerDirectives.transferToInterpreter();
var err = ctx.getBuiltins().error().makeTypeError(this.ensoType, value, type.getName());
throw new PanicException(err, this);
}
return type.cast(conv);
} else {
return type.cast(value);
}
}

public final Object processWarnings(VirtualFrame frame, Object result, ArgContext context) {
assert context.warnings != null;
if (this.appendWarningNode == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
this.appendWarningNode = insert(AppendWarningNode.build());
}
return appendWarningNode.executeAppend(frame, result, context.warnings);
}

abstract Object executeConversion(Object obj);

@Specialization
final Object extractMultiValue(EnsoMultiValue emv, @Cached EnsoMultiValue.CastToNode castTo) {
var extracted = castTo.findTypeOrNull(ensoType, emv, false, false);
return extracted;
}

@Fallback
final Object justReturnIt(Object obj) {
return obj;
}

public static ArgNode create(
boolean isSelf,
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
boolean requiresCast,
boolean checkErrors,
boolean checkPanicSentinel,
boolean checkWarnings) {
byte flags = 0x00;
if (isSelf) {
flags |= IS_SELF;
}
if (requiresCast) {
flags |= REQUIRES_CAST;
}
if (checkErrors) {
flags |= CHECK_ERRORS;
}
if (checkPanicSentinel) {
flags |= CHECK_PANIC_SENTINEL;
}
if (checkWarnings) {
flags |= CHECK_WARNINGS;
}
return BuiltinRootNodeFactory.ArgNodeGen.create(flags);
}
}

@SuppressWarnings("unchecked")
private static <E extends Exception> E raise(Class<E> clazz, Throwable t) throws E {
throw (E) t;
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package org.enso.interpreter.node.expression.builtin;

import org.enso.interpreter.dsl.BuiltinType;
import org.enso.interpreter.runtime.error.DataflowError;

@BuiltinType(name = "Standard.Base.Error.Error")
public class Error extends Builtin {
public final class Error extends Builtin {
public Error() {
super(DataflowError.class);
}

@Override
protected Class<? extends Builtin> getSuperType() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
import org.enso.interpreter.dsl.BuiltinType;

@BuiltinType(name = "Standard.Base.Nothing.Nothing")
public class Nothing extends Builtin {
public final class Nothing extends Builtin {
public Nothing() {
super(Void.class);
}

@Override
public boolean containsValues() {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@
import org.enso.interpreter.dsl.BuiltinType;

@BuiltinType
public class Polyglot extends Builtin {}
public final class Polyglot extends Builtin {
public Polyglot() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say we have way too many Builtins. Not sure why Polyglot or Debug should be builtin types at all!?

super(Object.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
public abstract class UniquelyConstructibleBuiltin extends Builtin {
private @CompilerDirectives.CompilationFinal AtomConstructor uniqueConstructor;

protected UniquelyConstructibleBuiltin() {
super(Object.class);
}

public final AtomConstructor getUniqueConstructor() {
return uniqueConstructor;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@
import org.enso.interpreter.node.expression.builtin.Builtin;

@BuiltinType
public class Debug extends Builtin {}
public class Debug extends Builtin {
public Debug() {
super(Object.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

@BuiltinType
public final class NumberParseError extends Builtin {
public NumberParseError() {
super(Object.class);
}

@Override
protected final List<Cons> getDeclaredConstructors() {
return List.of(new Cons("Error", "text"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@

import org.enso.interpreter.dsl.BuiltinType;
import org.enso.interpreter.node.expression.builtin.Builtin;
import org.enso.interpreter.runtime.error.PanicException;

@BuiltinType(name = "Standard.Base.Panic.Panic")
public class Panic extends Builtin {
public final class Panic extends Builtin {
public Panic() {
super(PanicException.class);
}

@Override
public boolean containsValues() {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
import org.enso.interpreter.runtime.data.atom.AtomConstructor;

@BuiltinType(name = "Standard.Base.Errors.Problem_Behavior.Problem_Behavior")
public class ProblemBehavior extends Builtin {
public final class ProblemBehavior extends Builtin {
public ProblemBehavior() {
super(Object.class);
}

@Override
protected List<Cons> getDeclaredConstructors() {
Expand Down
Loading
Loading