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

[Orc][examples] Fix lljit-with-remote-debugging test failure #74764

Closed
wants to merge 1 commit into from

Conversation

RoboTux
Copy link
Contributor

@RoboTux RoboTux commented Dec 7, 2023

Test lljit-with-remote-debugging fails on AArch64 because the
argc_sub1_elf.ll input file have a x86_64 target triple. Since there is
no way to set the target triple when invoking LLJITWithRemoteDebugging
let's require x86_64 target to be the default target.

Test lljit-with-remote-debugging fails on AArch64 because the
argc_sub1_elf.ll input file have a x86_64 target triple. Since there is
no way to set the target triple when invoking LLJITWithRemoteDebugging
let's require x86_64 target to be the default target.
@lhames
Copy link
Contributor

lhames commented Dec 7, 2023

@RoboTux @weliveindetail I don't think that argc_sub1_elf.ll contains anything x86-64 specific -- what if we just removed the triple instead?

@weliveindetail
Copy link
Contributor

what if we just removed the triple instead?

Yes, we could try that. Otherwise, running the test only on x86_64 would be fine too.

@RoboTux
Copy link
Contributor Author

RoboTux commented Dec 8, 2023

@RoboTux @weliveindetail I don't think that argc_sub1_elf.ll contains anything x86-64 specific -- what if we just removed the triple instead?

I tried but then it complains that there is no target "". Perhaps it's worth fixing orc to support an empty target.

weliveindetail added a commit that referenced this pull request Dec 8, 2023
…st (#74831)

#74764 reported that the
`lljit-with-remote-debugging` test fails on AArch64 hosts, because the
input IR file states arch x86_64 explicitly. In order to drop the target
triple we have to remove a check in the example implementation.

Not sure it's fully portable now, but at least it's better than before.
@RoboTux RoboTux closed this Dec 13, 2023
@RoboTux
Copy link
Contributor Author

RoboTux commented Dec 13, 2023

Dropping since #74831 has been merged.

@RoboTux RoboTux deleted the lljit_remove_debug_requirement branch December 13, 2023 09:50
@lhames
Copy link
Contributor

lhames commented Dec 18, 2023

@RoboTux @weliveindetail I don't think that argc_sub1_elf.ll contains anything x86-64 specific -- what if we just removed the triple instead?

I tried but then it complains that there is no target "". Perhaps it's worth fixing orc to support an empty target.

I missed this earlier -- that's a good idea. LLJIT seems like the right place to handle this: addIRModule can check for an empty triple and replace it with the triple reported through the ExecutionSession.

@RoboTux
Copy link
Contributor Author

RoboTux commented Dec 18, 2023

I missed this earlier -- that's a good idea. LLJIT seems like the right place to handle this: addIRModule can check for an empty triple and replace it with the triple reported through the ExecutionSession.

That's been done by Stefan in d86a937.

@lhames
Copy link
Contributor

lhames commented Dec 18, 2023

I missed this earlier -- that's a good idea. LLJIT seems like the right place to handle this: addIRModule can check for an empty triple and replace it with the triple reported through the ExecutionSession.

That's been done by Stefan in d86a937.

Stefan's change was to the testcase. I was proposing to change LLJIT itself, though now that I've taken a look I can see that it wouldn't have made a difference here: The testcase triple was being used to construct the target (via JITTargetMachineBuilder) and that's why the empty triple was causing a "no target" error. Normally the target is constructed using the ExecutionSession's target triple, and LLVM optimizations and CodeGen will happily process a module with an empty triple.

@RoboTux
Copy link
Contributor Author

RoboTux commented Dec 18, 2023

Stefan's change was to the testcase. I was proposing to change LLJIT itself, though now that I've taken a look I can see that it wouldn't have made a difference here: The testcase triple was being used to construct the target (via JITTargetMachineBuilder) and that's why the empty triple was causing a "no target" error. Normally the target is constructed using the ExecutionSession's target triple, and LLVM optimizations and CodeGen will happily process a module with an empty triple.

Oh right, I didn't realize LLJITWithRemoteDebugging is a test only binary. My bad.

@weliveindetail
Copy link
Contributor

I started drafting a PR with some maintenance on implementation and tests for advanced LLJIT examples here: #76236

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