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

Versions 6 and 7 progress thread #82

Open
stelleg opened this issue Jan 12, 2019 · 5 comments
Open

Versions 6 and 7 progress thread #82

stelleg opened this issue Jan 12, 2019 · 5 comments

Comments

@stelleg
Copy link
Collaborator

stelleg commented Jan 12, 2019

I thought I'd open an issue to discuss progress towards up to date versions 6 and 7. stelleg:release_60 is current master rebased on llvm:release_60 that is failing 5 tapir-specific tests, 2 loop fusion and 3 parallel-for loop vectorization tests. Once I've figured those out, I'll make a PR, and start rebasing that branch on llvm:release_70. I used some of the work from tapir-llvm:release_60 and master-v6 to help fix some of the issues rebasing on llvm:release_60. tapir-llvm:release_60 seems to have diverged slightly, making rebasing off of it more difficult than llvm:release_60. Let me know if you want any logic from tapir-llvm:release_60 incorporated before I make the PR.

@neboat
Copy link
Collaborator

neboat commented Jan 13, 2019

A minor point: tapir-llvm:release_60 is based on a release candidate of LLVM 6, whereas tapir-llvm:release_60-release is based on the actual LLVM 6.0.0 release. (Rebasing onto a release candidate was my mistake. Lesson learned; in the future, we'll avoid making public branches that are rebased on release candidates.)

There are several changes in tapir-llvm:release_60-release having to do with exceptions that you'll want to incorporate. The LLVM-5-era design for how Tapir integrates with exception-handling code is fundamentally wrong, which was the cause of issue #32. The changes in tapir-llvm:release_60 and tapir-llvm:release_60-release fix these problems by adding an optional "unwind" destination to detaches and introducing the "detached-rethrow" intrinsic. These changes affect a fair amount of the codebase, but I believe they are important changes, especially for dealing with C++ code. I would highly encourage incorporating these changes.

Although it's not as crucial, you might want to incorporate the TapirTaskInfo pass from the WIP-taskinfo branch as well. These changes deal make it easier for code elsewhere in the codebase to handle Tapir. For example, TapirTaskInfo encapsulates a lot of the ugly code that is needed for dealing with exceptions. I have a lot of confidence in these changes now, and I was planning to move them into tapir-llvm:release_60-release in the near future.

One last tip, if you want to find more bug fixes to incorporate, check out the Tapir regression tests in tapir-llvm:release_60 or tapir-llvm:release_60-release.

@stelleg
Copy link
Collaborator Author

stelleg commented Jan 22, 2019

Thanks TB.

I'm working on cherry-picking those changes. One other set of changes I noticed in release_60 were some changes that added cilk sanitizer and stealable intrinsics. Should those be pulled in as well?

@stelleg stelleg closed this as completed Jan 22, 2019
@stelleg stelleg reopened this Jan 22, 2019
@neboat
Copy link
Collaborator

neboat commented Jan 22, 2019

You mean "cilk sanitizer" and "stealable" function attributes? I think you'll want to pull those in as well, though I don't think they should be too much trouble.

We use the stealable attribute to fix some machine-code-generation issues for parallel programs. For Cilk codes, the work-stealing scheduler means that certain optimizations on x86 machine code are not legal to do on functions that might perform a cilk_spawn. (In particular, any such function in Cilk must be compiled to use %rbp to keep track of the function frame; it cannot use statically computable offsets from %rsp to keep track of the frame, because a worker that steals the frame will have a different stack, and thus a different %rsp.) The stealable attribute lets us keep track of these functions after Tapir lowering, so that machine-code gen can avoid performing those illegal optimizations on relevant functions. I imagine that similar issues might arise when dealing with other parallel backends, although I'm not sure I have a non-Cilk example handy.

My memory is fuzzy re the cilk-sanitizer attribute, but I think we use that attribute to make sure Cilksan won't ever instrument the same function twice. It's also handy for debugging Cilksan and making sure it instruments all relevant functions, as I recall.

@stelleg
Copy link
Collaborator Author

stelleg commented Jan 22, 2019

Whoops, yes I meant the function attributes. Good to know, I'll include them as well. Thanks.

@wsmoses
Copy link
Owner

wsmoses commented Jan 31, 2019

Also if possible we want to merge in the commits from the gpu WIP codebase as well. @stelleg can you push the branches to the main repo for the sake of testing CI (among other things)?

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

No branches or pull requests

3 participants