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

fix: direct call optional parameter function in more cases #2676

Closed

Conversation

HerrCai0907
Copy link
Member

stub varargs function can be optimized in those cases:

  1. operator +/- prefix expression
function foo(a: i32 = -1): void {}
  1. global variant, we can directly use global.get to get this value instead of use stub varargs.
let v = 1;
function foo(a: i32 = v): void {}
  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

src/ast.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

Note that unary prefix operators can be overloaded, in which case these won't compile to a const. The proposed changes might hence be problematic.

@MaxGraey
Copy link
Member

Note that unary prefix operators can be overloaded, in which case these won't compile to a const

In this case unaryPrefixExpr.operand.isLiteral() will return false due to operand will be reference, so I don't think this is a matter.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

Currently, references are just i32s, though. Imagine, for example

somefunc(!"somestring")

where !"somestring" compiles to

(call $String#__not
  (i32.const ...)
)

that not only checks whether the value is 0 but also whether the string has a zero length. Can become arbitrarily complex with usize wrappers like Pointer<T>.

@HerrCai0907
Copy link
Member Author

we cannot overload operator for literal type. And that is the reason I change the function name.
isLiteral is more strictly than compilesToConst. Actually we only want to match the simplest expression in this function.
So I think overload is not a problem.

@HerrCai0907
Copy link
Member Author

Looks like I need to revert the change for ! and ~. + and - is absolutely fine in here.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

For reference, here's what the compiler does for unary prefix + and -:

assemblyscript/src/compiler.ts

Lines 9372 to 9405 in 688746a

switch (expression.operator) {
case Token.Plus: {
expr = this.compileExpression(
expression.operand,
contextualType.exceptVoid,
Constraints.None
);
// check operator overload
let classReference = this.currentType.getClassOrWrapper(this.program);
if (classReference) {
let overload = classReference.lookupOverload(OperatorKind.Plus);
if (overload) return this.compileUnaryOverload(overload, expression.operand, expr, expression);
}
if (!this.currentType.isValue) {
this.error(
DiagnosticCode.The_0_operator_cannot_be_applied_to_type_1,
expression.range, "+", this.currentType.toString()
);
return module.unreachable();
}
// nop
break;
}
case Token.Minus: {
let operand = expression.operand;
if (operand.isNumericLiteral) {
// implicitly negate integer and float literals. also enables proper checking of literal ranges.
expr = this.compileLiteralExpression(<LiteralExpression>operand, contextualType, Constraints.None, true);
// compileExpression normally does this:
if (this.options.sourceMap) this.addDebugLocation(expr, expression.range);
break;
}

Note the checks for operator overloads, which might exist on wrapper classes, and that implicit negation is limited to numeric literals, excluding string literals.

@HerrCai0907
Copy link
Member Author

I don't find any overload in current implement of number wrapper class.
Is it possible to replace the implement of wrapper classes? I have tried `"use": "~lib/number/I32=assembly/index/I32_MOCK" but it does not work.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

My point is rather that the check becomes too brittle for my taste, as !"somestring" indicates, i.e. there are code paths that can technically break it. Say we would put a prominent warning there that this only works as long as operator assumptions hold (which we'll likely forget about), then the follow-up question naturally becomes whether the mechanism is worth it in the first place. Looking at release builds, where stub calls become inlined anyway, my immediate reflex is: Do we need this mechanism in the first place? (Do we?)

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Apr 24, 2023

unfortunately it cannot inline.

 (func $assembly/index/_start
  i32.const 1
  global.set $~argumentsLength
  i32.const 3
  call $assembly/index/foo@varargs
  i32.const 1
  global.set $~argumentsLength
  i32.const 4
  call $assembly/index/foo@varargs
 )
const t = -1;

export let v1 = 0;
export let v2 = "";

function foo(a: i32, b: i32 = t): void {
  v1 = a + b;
  v2 = a.toString() + b.toString();
}

export function _start(): void {
  foo(3);
  foo(4);
}

@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

True, looks like there it doesn't inline because other things have already been inlined into the stub, then becoming too big to be worth to inline. That's indeed unfortunate. I guess could be mitigated by generating multiple stubs, one per number of arguments, as that'd suppress the br_table while otherwise only moving where things are inlined concretely hmm.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 24, 2023

Perhaps as a compromise for the time being: Let's make compilesToConst also take the type, then only doing the the other operators if the type is numerical? Plus a comment so we don't forget?

@HerrCai0907 HerrCai0907 deleted the optimize-optional-args branch October 16, 2023 07:31
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

3 participants