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

Add stdlib test importing every stdlib module + fix stdlib #795

Merged
merged 7 commits into from
Jan 24, 2025
Merged

Conversation

jiribenes
Copy link
Contributor

@jiribenes jiribenes commented Jan 23, 2025

Resolves #614 by adding a stdlib test that imports every single (common) stdlib module.

Let's add ACME (in our context "All Common Modules in Effekt" :)) to the stdlib tests: just import and compile all modules.

This revealed 3 different bugs in stdlib + codegen bugs on LLVM (moved to #801).

@jiribenes
Copy link
Contributor Author

jiribenes commented Jan 23, 2025

Ideal follow-up: make CI so that on change in /libraries/common/, we check that the acme.effekt file contains every module in that folder, recursively (just generate it again and diff). If not, fail and spam the PR.
EDIT: Done. ACME CI now exists.

@jiribenes
Copy link
Contributor Author

jiribenes commented Jan 23, 2025

Note

Moved to #801

New LLVM bugs, cc @phischu:

$ EFFEKT_DEBUG=1 effekt --backend=llvm --ir-write-all --debug --optimize examples/stdlib/json.effekt
opt: ./out/json.ll:4001:9: error: multiple definition of local value named 'd_24_132_3_22162'
        %d_24_132_3_22162 = insertvalue %Neg %vtable_temporary_865, %Object %closure_861, 1
        ^

$ EFFEKT_DEBUG=1 effekt --backend=llvm --ir-write-all --debug --optimize examples/stdlib/buffer.effekt
Instruction does not dominate all uses!
  %buffer_24_4387 = insertvalue %Neg %vtable_temporary_1257, ptr %closure_1239, 1
  call void @shareNegative(%Neg %buffer_24_4387)
Instruction does not dominate all uses!
  %buffer_24_4387 = insertvalue %Neg %vtable_temporary_1257, ptr %closure_1239, 1
  store %Neg %buffer_24_4387, ptr %buffer_24_4387_pointer_1255, align 8, !noalias !0
Instruction does not dominate all uses!
  %buffer_24_4387 = insertvalue %Neg %vtable_temporary_1257, ptr %closure_1239, 1
  call void @eraseNegative(%Neg %buffer_24_4387)
opt: ./out/buffer.ll: error: input module is broken!

EDIT: I think it's the same problem, actually.
For negative types, we generate:

define tailcc void @xyz_clause_1(...) { ... }
define tailcc void @xyz_clause_2(...) { ... }

which always share what would be:

; new xyz_clause_6769, 0 parameters
        %xyz_6769_pointer_492 = getelementptr {%reference, %reference, %neg, %pos, i64}, %environment %environment_489, i64 0, i32 2
        %xyz_6769 = load %neg, ptr % xyz_6769_pointer_492, !noalias !2

so the same value is defined under the same name in multiple different functions.

val result: T = contents.remove(head).getOrElse { <> };
val result: T = contents.unsafeGet(head);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is semantically correct, we use it as a circular buffer, so we don't really need to invalidate the previous value -- we'd have overwritten it.

@jiribenes jiribenes removed area:llvm blocked Blocked on other issues / PRs labels Jan 24, 2025
@jiribenes jiribenes changed the title Add stdlib test importing every stdlib module Add stdlib test importing every stdlib module + fix stdlib Jan 24, 2025
Copy link
Contributor Author

@jiribenes jiribenes left a comment

Choose a reason for hiding this comment

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

I'll merge this since it's a change that I really want in the next release, any nitpicks can be fixed in a separate PR :shipit:

@jiribenes jiribenes merged commit 10973e4 into master Jan 24, 2025
3 checks passed
@jiribenes jiribenes deleted the acme branch January 24, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a test that loads all stdlib files
1 participant