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

WIP: Gc addressspace 0 #1193

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft

WIP: Gc addressspace 0 #1193

wants to merge 9 commits into from

Conversation

mariaKt
Copy link
Contributor

@mariaKt mariaKt commented Dec 19, 2024

This PR adds

  • the explicit use/casting to address space 0 in CreateTerm.cpp.
  • the infrastructure to define the functions that contain the addrspacecast instructions for the various address spaces (a separate file, linked after the regular passes, and the always inline pass executed afterwards).

Current status: The tests work as expected when the conditional compilation flag use_gcstrategy that enables the use of the LLVM backend strategy is disabled.

Enabling the flag, e.g. by adding the tests

// RUN: %gcs-interpreter
// RUN: %check-grep

in defn/imp.kore, results in the test failing (that interpreter fails with segfault).

This is WIP, and is merged in order to keep the code up to date until we revisit this, to

  • address the segfault
  • replace address space 0 with the actual address space needed in each place
  • similarly edit the other files

- Data layout and target triple configured.
- Temporarily comment out the addrspacecast command (cannot cast to same AS).
@mariaKt mariaKt requested a review from dwightguth December 19, 2024 22:20
Copy link
Collaborator

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

I haven't fully reviewed this yet, but I have a couple comments. For the purposes of clarity, can you share the .ll file of IMP that this PR generates?

tools/llvm-kompile-codegen/main.cpp Show resolved Hide resolved
Copy link
Collaborator

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

it's possible the segfault is due to doing an addrspace cast from an address space to itself, which the LLVM documentation tells you not to do. I'll see if I can spot any other possible causes when you share the llvm bitcode I asked you for.

@mariaKt
Copy link
Contributor Author

mariaKt commented Dec 20, 2024

Here is the ll for IMP.
imp.ll.txt

@dwightguth
Copy link
Collaborator

It would help if you could do the following:

  1. Determine whether or not the bug still occurs in the version of IMP that does not have the pattern matching optimization enabled.
  2. Share the version of the code generated with --no-optimize.

@dwightguth
Copy link
Collaborator

Something feels weird about the code, but I don't immediately know what it is and I could be seeing something that is actually normal.

There's a third thing you should try:

  • compare the generated code before and after making this change and see if anything seems obviously suspect in the difference between the two on the same function.

@mariaKt
Copy link
Contributor Author

mariaKt commented Dec 20, 2024

  1. imp.kore also failed when disabling pattern matching optimization.
  2. I attach the code generated with --nooptimize (pattern matching optimization enabled again).
    imp.gc.patmatchopt.noopt.ll.txt
  3. I will compare the ll files for IMP from develop and this branch. I attach them as well.
    imp.develop.patmatchopt.opt.ll.txt
    imp.gc.patmatchopt.opt.ll.txt

@mariaKt mariaKt changed the title Gc addressspace WIP: Gc addressspace 0 Jan 13, 2025
@mariaKt mariaKt requested review from dwightguth and theo25 January 13, 2025 23:03
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.

2 participants