-
Notifications
You must be signed in to change notification settings - Fork 144
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
Static-link Legion into compiled Regent programs #341
base: master
Are you sure you want to change the base?
Conversation
Adding @streichler for review as well. I'm fine with this change as it seems like static linking libraries is necessary for some machines like Titan (although we've found we can cheat in some circumstances too). @manopapad What prevents this from passing the test suite right now? |
Please don't merge this yet. There are two different things going on here, and I want to sort them out. One issue is that Cray systems don't like dynamic libraries. In theory, I'm fine with static linking Regent applications. My main objection is that it doesn't actually fix @manopapad's issue; Cray doesn't like dynamic libraries but it does allow them, and I've been running extensively on Titan in this way. The actual issue that is motivating this patch is a link error on Titan. The fix is the second part of this PR: dumping linker flags to a file in For the moment, I think I can fix the second issue by teaching Regent's While tangentially related, it would also be nice to do some cleanup of the build process in general. E.g. if we could teach the Makefile to build a |
For reference, for most Legion-only applications right now we actually already build liblegion.a and librealm.a (or liblegion.so and librealm.so) libraries and then link them agains the application code, so I think moving to a model where we have liblegion and librealm separate from liblegion_terra (maybe rename to libregent) should be just fine. In terms of keeping things in-sync for linker flags, I at least would be alright adding some runtime support to provide a string of linker flags that were set when the runtime was compiled. This would stay consistent wherever a particular build of the runtime is used and would be updated appropriately whenever the runtime is rebuilt. I'm guessing this needs to be a Legion thing since it will need to know about both Legion and Realm libraries. |
To summarize the discussion on this PR, here's a list of (orthogonal) improvements that have been suggested so far:
At this point I'm happy to reduce the scope of this PR to a subset of the above features, or close it and tackle each feature in a separate issue/PR. Awaiting instructions. |
|
Re: 3. I'd prefer that this be done via a constant or a define in
or
We'll have to test which of these Terra actually supports. |
Re: 1. No, our C++ codes don't work like this right now. We create Re: 2. I'm fine with this and think the patch already basically solves this. We can rewrite the timing functions in Terra if necessary. Re: 4. I think this gets easier if we factor out (3), so I'm also probably fine with this in practice. Given the separate concerns, I'd be happier accepting one patch at a time. Part (2) can basically go now. The other parts are probably not that bad, but we want to make sure we do them right. |
I've been doing some refactoring of the code such that Regent now has support for CMake. With CMake, Regent already has support for (1). Item (3) would also be straightforward since CMake already generates a file called |
@elliottslaughter, what's the plan for this PR? |
I think pieces of this PR may still be useful, but we're probably not going to merge it as-is. Some of this would be easier if we were willing to declare CMake the official build system. As it is there will be additional hacks (beyond what is in this PR) needed to implement some of the proposed changes. I've also thought about some of the points and think we should revisit some of the implementation details, but that can be discussed if we decide to move forward with this. I think the bottom line is that it's not super high priority so no one's been giving it the time it needs to push all the way through. |
@elliottslaughter another poke on this - this PR appears to be bit-rotting. Do we want to clean it up, or close it and replace with a more general RFE, or just close it with prejudice? |
I guess I stand by what I said before. This PR really needs a champion to figure out what to keep and what to cut and what needs to be reworked. It's not high priority enough to have gotten much attention, but it's still valuable enough I think we wouldn't want to toss it completely. (Though, I suppose if it continues to bitrot we might eventually get to the point of tossing it and recreating it rather than slicing and rebasing or whatever.) |
This is really a proof-of-concept. There's definitely a cleaner way to propagate linking information to the Regent compiler. Also, this won't pass the test suite as-is.
With these changes, circuit (with SAVEOBJ=1) and soleil-x (without a custom mapper) compile and run correctly, and without dynamic-linking liblegion_terra.so. I've tested this locally and on Titan.
I've tried to avoid dynamic-linking during compilation as well. This should only be necessary if you want to run the code on-the-fly instead of emitting an executable (which is why I do terralib.linklibrary in regentlib.start), or want to use functions from Legion during compilation (which is what happens in the test suite, and in -fdebug 1 mode, both of which use at least the timer code from Legion).