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 optimisations with compileExprIndexExpr #1135

Closed
wants to merge 4 commits into from

Conversation

Lonegwadiwaitor
Copy link

@Lonegwadiwaitor Lonegwadiwaitor commented Dec 23, 2023

Currently in Luau 0.607's compileExprIndexExpr:

Constant cv = getConstant(expr->index);

always returns nil, meaning the conditions that generate LOP_GETTABLEN and LOP_GETTABLEKS for string/number indexes are both unreachable, this has been fixed in this PR.

LOP_GETIMPORT can also now be generated for string indexes if optimisations are enabled.

Comparisons

Test 1 (unreachable string/number branches in compileExprIndexExpr)

Repro code:

local _ = foo["bar"]

Luau 0.607 with no optimisations
(output from luau.lonegladiator.dev, same output as luau-compile util):
image
PR with no optimisations:
image

Test 2 (LOP_GETIMPORT for compileExprIndexExpr)

Repro code:

local _ = foo["bar"]

Luau 0.607 with -O1 and -O2:
image
PR with -O1 and -O2:
image

Copy link

@matthargett matthargett left a comment

Choose a reason for hiding this comment

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

Can you add unit tests? There are examples of opcode emission tests in the codebase.

@Lonegwadiwaitor
Copy link
Author

Lonegwadiwaitor commented Dec 24, 2023

Modified import dumping logic in BytecodeBuilder::dumpConstant to wrap brackets around indexes with whitespace as it looked a little silly without it.

2 new test cases: IndexExprConditions & IndexExprImport

IndexExprConditions

Makes sure that GETTABLEKS is being generated for string indexes and GETTABLEN for number indexes when no optimisations are enabled.

IndexExprImport

tests GETIMPORT for compileExprIndexExpr

@vegorov-rbx
Copy link
Collaborator

vegorov-rbx commented Jan 8, 2024

Missing optimizations are not bugs, please fix the PR title.

For GETTABLEN and GETTABLEKS:

always returns nil

This is simply not true, constants are filled in optimization level 1.
If you need optimizations to happen, enable optimizations.
Additionally, your change breaks the constant propagation in the indexer expression.

For the getimport part, your code introduces too much code duplication and once again, breaks the computed constants on O1.
Motivation for a change is missing a bit, since you can rewrite your foo["bar"] into foo.bar, but this is still a good idea overall, so can be rewritten.

Changes are also not flagged.

The change for check if index contains whitespace is unnecessary and smells of buffer overflows since a string view like StringRef is checked with a C functions that's looking for null-termination.
It is also incomplete because there are many other cases of AstExprIndexExpr strings that are not valid in a.b.

Thank you for your interest in improving Luau, but we are not going to accept the PR in current form.

@Lonegwadiwaitor Lonegwadiwaitor changed the title Fix optimisation-related bugs with compileExprIndexExpr Fix optimisations with compileExprIndexExpr Jan 8, 2024
@Lonegwadiwaitor Lonegwadiwaitor deleted the master branch January 16, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants