Add Polly support #50044

DiamondLovesYou · 2018-04-18T07:46:17Z

It is always compiled and linked, but not used by default.

Use can be triggered via -Z polly=true or at compiler build-time (see config.toml.example).

kennytm · 2018-04-18T21:13:41Z

src/librustc/session/config.rs

@@ -1074,6 +1074,9 @@ options! {CodegenOptions, CodegenSetter, basic_codegen_options,
        [TRACKED], "panic strategy to compile crate with"),
    incremental: Option<String> = (None, parse_opt_string, [UNTRACKED],
          "enable incremental compilation"),
+    polly: bool = (option_env!("RUSTC_FORCE_USE_POLLY").map(|_| true ).unwrap_or(false),


Could you make this flag -Z polly instead of -C polly? We'll likely not insta-stabilize this flag.

BTW, x.map(|_| true).unwrap_or(false) is equivalent to x.is_some().

It's a codegen option though, to me it doesn't make sense to put it in with debugging options. I've moved it anyway, but yeah.

Second point is a good one, not sure what I was thinking on that.

gnzlbg · 2018-04-19T12:06:20Z

This would close #39884 :)

So thanks!

kennytm · 2018-04-19T20:32:40Z

Assuming the error in #50044 (comment) does not affect LLVM 6, I'd like to check if polly fixes #45222.

@bors try

bors · 2018-04-19T20:32:52Z

⌛ Trying commit 7762503 with merge af6e74d...

Add Polly support It is always compiled and linked, but not used by default. Use can be triggered via `-C polly=true` or at compiler build-time (see `config.toml.example`).

bors · 2018-04-19T21:01:05Z

💔 Test failed - status-travis

kennytm · 2018-04-19T22:25:48Z

src/librustc/session/config.rs

@@ -1304,6 +1304,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
        "tell the linker to strip debuginfo when building without debuginfo enabled."),
    share_generics: Option<bool> = (None, parse_opt_bool, [TRACKED],
          "make the current crate share its generic instantiations"),
+    polly: bool = (option_env!("RUSTC_FORCE_USE_POLLY").is_some(),


I'd like a clarification of use of option_env!() here.

option_env!() is evaluated at compile time. This means if we set use-polly-by-default = true in config.toml, the produced rustc will always use Polly. Is this the intended behavior?

$ echo 'use-polly-by-default = true' >> config.toml $ ./x.py build ... $ rustc +stage2 x.rs ##^ will compile x.rs with -Zpolly even without RUSTC_FORCE_USE_POLLY=1

@DiamondLovesYou Could you put the explanation into config.toml.example and also above this line in src/librustc/session/config.rs? Thanks.

(Also, the code still fails to link.)

Sure. Re linking issue, yep. I failed to consider using the system LLVM, and am working on it (not super trivial, rustllvm can't initialize polly's passes, for example).

I suppose I should mention that the reason use-polly-by-default exists is so that one can run all the tests with polly enabled.

Not sure if there is a better way...

@DiamondLovesYou You could also modify the bootstrap rustc (src/bootstrap/bin/rustc.rs) to always pass -Z polly when RUSTC_FORCE_USE_POLLY is set.

bors · 2018-04-24T10:51:42Z

☔ The latest upstream changes (presumably #49837) made this pull request unmergeable. Please resolve the merge conflicts.

kennytm · 2018-05-01T08:31:24Z

@bors try

bors · 2018-05-01T08:31:35Z

⌛ Trying commit 337b28c with merge 16cc121...

Add Polly support It is always compiled and linked, but not used by default. Use can be triggered via `-Z polly=true` or at compiler build-time (see `config.toml.example`).

bors · 2018-05-01T10:23:13Z

☀️ Test successful - status-travis
State: approved= try=True

DiamondLovesYou · 2018-05-01T15:22:33Z

I've made a few changes. The use of polly is now not tied to the compiler's build time configuration.

kennytm · 2018-05-01T19:39:59Z

src/bootstrap/compile.rs

 }

-pub fn rustc_cargo_env(builder: &Builder, cargo: &mut Command) {
+pub fn rustc_cargo_env(builder: &Builder, cargo: &mut Command, is_test: bool) {


Instead of modifying rustc_cargo_env, which forces one to pass a magic boolean everywhere, I suggest you modify the Builder::cargo function (in builder.rs) such that rust_polly_tests is used when cmd is "test" or "bench".

Also, why the rust_polly_tests is used (is_test == true) for "check" and "doc" in the current implementation?

Instead of modifying rustc_cargo_env, which forces one to pass a magic boolean everywhere, I suggest you modify the Builder::cargo function (in builder.rs) such that rust_polly_tests is used when cmd is "test" or "bench".

Ok, that sounds better.

Also, why the rust_polly_tests is used (is_test == true) for "check" and "doc" in the current implementation?

I want to make sure all tests are run with the polly optimization, to guard against possible polly bugs.

I want to make sure all tests are run with the polly optimization, to guard against possible polly bugs.

No tests would be run with cargo check and cargo doc though. Maybe "test" isn't a good name for this configuration 🤔.

kennytm · 2018-05-01T19:42:37Z

src/librustc_llvm/build.rs

+
+        // We can't specify the full libname to rust, so the linker will always expect (on unix)
+        // LLVMPolly to be libLLVMPolly, which won't be present. I didn't realize this fact until
+        // after I won't the following, but maybe this issue will be resolved in the future.


I think there's some typo in this comment? 😄

Whoops, you right.

…uses an LLVM which includes polly. Force LLVM rebuild on buildbots.

DiamondLovesYou · 2018-05-02T15:07:21Z

I've removed and cleaned up usage of that is_test variable. Should be ready to go.

kennytm · 2018-05-02T16:00:34Z

src/bootstrap/compile.rs

@@ -553,6 +553,18 @@ pub fn rustc_cargo_env(builder: &Builder, cargo: &mut Command) {
    if builder.config.rustc_parallel_queries {
        cargo.env("RUSTC_PARALLEL_QUERIES", "1");
    }
+
+    let use_polly = match builder.kind {


builder.kind only records how x.py is invoked, i.e. if you use

./x.py test src/test/compile-fail ...

then librustc etc will be built with builder.config.rust_polly_tests instead of builder.config.rust_polly_self, even if it is a build artifact.

ollie27 · 2018-05-03T11:25:27Z

src/bootstrap/bootstrap.py

@@ -690,6 +690,10 @@ def update_submodules(self):
            recorded_submodules[data[3]] = data[2]
        for module in filtered_submodules:
            self.update_submodule(module[0], module[1], recorded_submodules)
+        polly_path = "src/llvm/tools/polly"
+        if not os.path.exists(os.path.join(self.rust_root, polly_path)):
+            run(["git", "clone", "https://github.com/llvm-mirror/polly.git",


Can this not be a normal submodule placed in src/polly for example? I believe you can use LLVM_EXTERNAL_POLLY_SOURCE_DIR to tell llvm where to find it.

bors · 2018-05-05T21:18:58Z

☔ The latest upstream changes (presumably #50276) made this pull request unmergeable. Please resolve the merge conflicts.

pietroalbini · 2018-05-14T10:09:19Z

Ping from triage @DiamondLovesYou! There are some comments you should address.

pietroalbini · 2018-05-21T10:53:16Z

Thank you for this PR @DiamondLovesYou! Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

gnzlbg · 2018-05-21T13:17:58Z

Please don’t let this PR be forgotten. Maybe I can pick it up, what was missing?

…

On Mon 21. May 2018 at 12:53, Pietro Albini ***@***.***> wrote: Closed #50044 <#50044>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#50044 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NptWBFabNyAPkPA1azG4V6YkJwgUcks5t0pzDgaJpZM4TZk7P> .

kennytm · 2018-05-21T13:41:03Z

@gnzlbg #50044 (review) and #50044 (review) and rebase on top of latest master.

DiamondLovesYou · 2018-05-25T15:47:16Z

Sorry, I was AFK for a while. How does one reopen a PR?

kennytm · 2018-05-25T16:02:15Z

@DiamondLovesYou You'll need to open a new PR since you've force-pushed the polly branch.

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from 84ed5d9 to bedcc10 Compare April 18, 2018 08:53

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from bedcc10 to 7762503 Compare April 18, 2018 18:52

This comment has been minimized.

Sign in to view

kennytm reviewed Apr 18, 2018

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2018

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2018

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from 7762503 to 3df0c4b Compare April 19, 2018 21:17

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from 3df0c4b to d95ca1a Compare April 19, 2018 21:29

This comment has been minimized.

Sign in to view

kennytm reviewed Apr 19, 2018

View reviewed changes

kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2018

pietroalbini assigned kennytm Apr 20, 2018

DiamondLovesYou force-pushed the polly branch from d95ca1a to 337b28c Compare April 29, 2018 17:34

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2018

DiamondLovesYou force-pushed the polly branch from 337b28c to ea86cc7 Compare May 1, 2018 15:21

kennytm mentioned this pull request May 1, 2018

Big performance problem with closed intervals looping #45222

Open

kennytm reviewed May 1, 2018

View reviewed changes

DiamondLovesYou force-pushed the polly branch from ea86cc7 to c583556 Compare May 1, 2018 23:31

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from c583556 to 0b7b7c6 Compare May 2, 2018 01:04

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch 2 times, most recently from 92cc74e to b45133e Compare May 2, 2018 01:56

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from b45133e to c35cfc5 Compare May 2, 2018 02:02

This comment has been minimized.

Sign in to view

DiamondLovesYou force-pushed the polly branch from c35cfc5 to 5c61d69 Compare May 2, 2018 02:12

Add Polly support. Use can be triggered via -Z polly, when rustc …

4bcf25a

…uses an LLVM which includes polly. Force LLVM rebuild on buildbots.

DiamondLovesYou force-pushed the polly branch from 5c61d69 to 4bcf25a Compare May 2, 2018 02:17

kennytm reviewed May 2, 2018

View reviewed changes

ollie27 reviewed May 3, 2018

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2018

pietroalbini closed this May 21, 2018

kennytm mentioned this pull request May 25, 2018

Add Polly support. #51061

Closed

Add Polly support #50044

Add Polly support #50044

Conversation

DiamondLovesYou commented Apr 18, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg commented Apr 19, 2018 • edited Loading

kennytm commented Apr 19, 2018

bors commented Apr 19, 2018

bors commented Apr 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kennytm Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 24, 2018

kennytm commented May 1, 2018

bors commented May 1, 2018

bors commented May 1, 2018

DiamondLovesYou commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DiamondLovesYou commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 5, 2018

pietroalbini commented May 14, 2018

pietroalbini commented May 21, 2018

gnzlbg commented May 21, 2018 via email

kennytm commented May 21, 2018

DiamondLovesYou commented May 25, 2018

kennytm commented May 25, 2018

DiamondLovesYou commented Apr 18, 2018 •

edited

Loading

gnzlbg commented Apr 19, 2018 •

edited

Loading

kennytm Apr 19, 2018 •

edited

Loading