search

wsmoses / Tapir-LLVM

Tapir extension to LLVM for optimizing Parallel Programs
Other
131 stars 24 forks source link

Versions 6 and 7 progress thread #82

Open stelleg opened 5 years ago

stelleg commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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?

neboat commented 5 years ago

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 commented 5 years ago

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

wsmoses commented 5 years ago

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)?