-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add Polly support. #51061
Add Polly support. #51061
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
Previous discussion: #50044. |
@bors r+ |
📌 Commit 27586fd has been approved by |
cc #39884 |
⌛ Testing commit 27586fd2d4e1db8e30caf7423406ca18c50b14e5 with merge 15d620c54f78dcc994cb706abf7be4b2b0c8ba03... |
💔 Test failed - status-appveyor |
It caused a bunch on undefined references when linking
|
Huh, that's odd. Rust shouldn't ever try to link polly dynamically, due to the fact that it's in |
☔ The latest upstream changes (presumably #50955) made this pull request unmergeable. Please resolve the merge conflicts. |
6e97e37
to
0225bbd
Compare
Patch to make Polly not try to build the loadable modules on all Windows platforms: https://reviews.llvm.org/D51904. @mati865 I made a minor change: I used |
@DiamondLovesYou MSYS2 builds LLVM with As long as Rust doesn't enable |
@mati865 Okay, I dug in and managed to (just now) fix the original issue. I built LLVM with Sadly, using |
@DiamondLovesYou nice work but maybe you are going too deep? This is pretty much uncharted territory. |
@mati865 Maybe, but it doesn't matter now. New patch: https://reviews.llvm.org/D51963. |
☔ The latest upstream changes (presumably #54146) made this pull request unmergeable. Please resolve the merge conflicts. |
@DiamondLovesYou You're an absolute hero for pushing this through, thank you so much! Hope to see those Polly patches landing soon. |
Thanks! Hopefully I can actually get it in; a lot of platform specific issues are coming up, which are breaking |
…uses an LLVM which includes polly. Force LLVM rebuild on buildbots.
0225bbd
to
d51fa91
Compare
[submodule "src/polly"] | ||
path = src/polly | ||
url = https://github.com/llvm-mirror/polly.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rust-lang
should have its own fork of this repo?
I could imagine that if upstream polly gets synced with upstream llvm but rustc uses rust-lang-llvm with llvm upstream polly, there might be conflicts between let's say polly 8.0 and llvm 7.0 (the rustc-lang fork).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could track the release_70
branch instead of master
if version incompatibility is the only concern.
☔ The latest upstream changes (presumably #55230) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage, @kennytm: should this PR be closed? @DiamondLovesYou any updates? |
@Centril this was blocked by several patched on LLVM. If OP did not have any updates this could be closed as S-blocked-closed. |
Alright; closing as |
Enable LLVM Polly via llvm-args. I think doing it this way is better than in rust-lang#51061. Polly has other useful options and we probably don't want to create a `-Z` flag for each one of them. ![results](https://user-images.githubusercontent.com/7283601/97695555-338f7180-1adf-11eb-82bd-5130e0e6fa89.png) [Benchmark](https://gist.github.com/JRF63/9a6268b91720958e90dbe7abffe20298) I noticed that `-lto` seems to interfere with polly in this specific microbenchmark, as enabling it causes the perf to drop to that of non-polly builds. Other related PRs: rust-lang#75615
Use can be triggered via
-Z polly
, whenrustc
uses an LLVM which includes polly.Force LLVM rebuild on buildbots.