polymorphize: remove predicate logic #75737

davidtwco · 2020-08-20T10:45:23Z

This PR removes all logic which marks parameters as used based on their presence in predicates - given #75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped.

~~Opened as a draft as it depends on #75675 (will need rebased to re-enable a test).~~

r? @eddyb

src/test/ui/polymorphization/symbol-ambiguity.rs

eddyb · 2020-08-23T07:53:16Z

r=me once #75675 lands

davidtwco · 2020-10-16T10:57:12Z

Rebased after #75675 having landed, src/test/ui/polymorphization/symbol-ambiguity.rs passes after the ignore-test was removed, but I'm seeing a fun failure that I didn't before (when polymorphization is enabled):

---- [ui] ui/impl-trait/example-calendar.rs stdout ----

error: test compilation failed although it shouldn't!
status: signal: 6
command: "/home/david/Projects/rust/rust7/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/home/david/Projects/rust/rust7/src/test/ui/impl-trait/example-calendar.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/home/david/Projects/rust/rust7/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/example-calendar/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/home/david/Projects/rust/rust7/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/home/david/Projects/rust/rust7/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/example-calendar/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
rustc: /home/david/Projects/rust/rust7/src/llvm-project/llvm/include/llvm/IR/Instructions.h:1181: void llvm::ICmpInst::AssertOK(): Assertion `getOperand(0)->getType() == getOperand(1)->getType() && "Both operands to ICmp instruction are not of the same type!"' failed.

------------------------------------------



failures:
    [ui] ui/impl-trait/example-calendar.rs

This commit removes all logic which marks parameters as used based on their presence in predicates - given rust-lang#75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped. Signed-off-by: David Wood <david@davidtw.co>

Dylan-DPC-zz · 2020-11-24T16:19:29Z

@davidtwco is this ready for review or are we waiting for more changes?

davidtwco · 2020-11-24T16:27:17Z

@davidtwco is this ready for review or are we waiting for more changes?

I want to fix the test failure when polymorphization is enabled before landing this, just haven't had time yet.

oli-obk · 2020-12-28T11:01:58Z

r? @oli-obk

davidtwco · 2021-01-13T22:44:37Z

For onlookers: I've noticed the same test failure from this PR in #75414 which suggests that it isn't related to the contents of this PR. I'll still be looking into it before landing this.

bors · 2021-03-05T14:03:54Z

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

Dylan-DPC-zz · 2021-03-08T22:17:24Z

@davidtwco if you can resolve the conflict, we can push this towards a merge

JohnCSimon · 2021-05-17T02:56:11Z

ping from triage:
@davidtwco if you can resolve the conflict, we can push this towards a merge

camelid · 2021-09-02T22:02:36Z

triage: @davidtwco what are the next steps for this PR? It seems like it needs a rebase and you wanted to fix the -Zpolymorphize=on test failure? Thanks!

davidtwco · 2021-10-04T09:57:01Z

Closing, superseded by #89514.

…edicates, r=lcnr polymorphization: shims and predicates Supersedes rust-lang#75737 and rust-lang#75414. This pull request includes up some changes to polymorphization which hadn't landed previously and gets stage2 bootstrapping and the test suite passing when polymorphization is enabled. There are still issues with `type_id` and polymorphization to investigate but this should get polymorphization in a reasonable state to work on. - rust-lang#75737 and rust-lang#75414 both worked but were blocked on having the rest of the test suite pass (with polymorphization enabled) with and without the PRs. It makes more sense to just land these so that the changes are in. - rust-lang#75737's changes remove the restriction of `InstanceDef::Item` on polymorphization, so that shims can now be polymorphized. This won't have much of an effect until polymorphization's analysis is more advanced, but it doesn't hurt. - rust-lang#75414's changes remove all logic which marks parameters as used based on their presence in predicates - given rust-lang#75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped. - Polymorphization now explicitly checks (and skips) foreign items, this is necessary for stage2 bootstrapping to work when polymorphization is enabled. - The conditional determining the emission of a note adding context to a post-monomorphization error has been modified. Polymorphization results in `optimized_mir` running for shims during collection where that wouldn't happen previously, some errors are emitted during `optimized_mir` and these were considered post-monomorphization errors with the existing logic (more errors and shims have a `DefId` coming from the std crate, not the local crate), adding a note that resulted in tests failing. It isn't particularly feasible to change where polymorphization runs or prevent it from using `optimized_mir`, so it seemed more reasonable to not change the conditional. - `characteristic_def_id_of_type` was being invoked during partitioning for self types of impl blocks which had projections that depended on the value of unused generic parameters of a function - this caused a ICE in a debuginfo test. If partitioning is enabled and the instance needs substitution then this is skipped. That test still fails for me locally, but not with an ICE, but it fails in a fresh checkout too, so 🤷‍♂️. r? `@lcnr`

…icates, r=lcnr polymorphization: shims and predicates Supersedes rust-lang#75737 and rust-lang#75414. This pull request includes up some changes to polymorphization which hadn't landed previously and gets stage2 bootstrapping and the test suite passing when polymorphization is enabled. There are still issues with `type_id` and polymorphization to investigate but this should get polymorphization in a reasonable state to work on. - rust-lang#75737 and rust-lang#75414 both worked but were blocked on having the rest of the test suite pass (with polymorphization enabled) with and without the PRs. It makes more sense to just land these so that the changes are in. - rust-lang#75737's changes remove the restriction of `InstanceDef::Item` on polymorphization, so that shims can now be polymorphized. This won't have much of an effect until polymorphization's analysis is more advanced, but it doesn't hurt. - rust-lang#75414's changes remove all logic which marks parameters as used based on their presence in predicates - given rust-lang#75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped. - Polymorphization now explicitly checks (and skips) foreign items, this is necessary for stage2 bootstrapping to work when polymorphization is enabled. - The conditional determining the emission of a note adding context to a post-monomorphization error has been modified. Polymorphization results in `optimized_mir` running for shims during collection where that wouldn't happen previously, some errors are emitted during `optimized_mir` and these were considered post-monomorphization errors with the existing logic (more errors and shims have a `DefId` coming from the std crate, not the local crate), adding a note that resulted in tests failing. It isn't particularly feasible to change where polymorphization runs or prevent it from using `optimized_mir`, so it seemed more reasonable to not change the conditional. - `characteristic_def_id_of_type` was being invoked during partitioning for self types of impl blocks which had projections that depended on the value of unused generic parameters of a function - this caused a ICE in a debuginfo test. If partitioning is enabled and the instance needs substitution then this is skipped. That test still fails for me locally, but not with an ICE, but it fails in a fresh checkout too, so 🤷‍♂️. r? `@lcnr`

rust-highfive assigned eddyb Aug 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2020

davidtwco commented Aug 20, 2020

View reviewed changes

src/test/ui/polymorphization/symbol-ambiguity.rs Outdated Show resolved Hide resolved

davidtwco mentioned this pull request Aug 20, 2020

mangling: mangle impl params w/ v0 scheme #75675

Merged

eddyb approved these changes Aug 23, 2020

View reviewed changes

eddyb added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2020

This comment has been minimized.

Sign in to view

davidtwco force-pushed the polymorphization-remove-predicate-logic branch from f9a6297 to 6b13e61 Compare August 30, 2020 18:25

davidtwco force-pushed the polymorphization-remove-predicate-logic branch from 6b13e61 to 59a9909 Compare October 16, 2020 10:56

davidtwco marked this pull request as ready for review October 16, 2020 10:56

This comment has been minimized.

Sign in to view

davidtwco force-pushed the polymorphization-remove-predicate-logic branch from 59a9909 to b520bba Compare November 2, 2020 11:53

davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 8, 2020

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2020

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2020

crlf0710 marked this pull request as draft December 11, 2020 12:26

rust-highfive assigned oli-obk and unassigned eddyb Dec 28, 2020

oli-obk marked this pull request as ready for review January 14, 2021 08:04

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 12, 2021

camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2021

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2021

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2021

davidtwco mentioned this pull request Oct 4, 2021

polymorphization: shims and predicates #89514

Merged

davidtwco closed this Oct 4, 2021

davidtwco deleted the polymorphization-remove-predicate-logic branch October 5, 2021 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

polymorphize: remove predicate logic #75737

polymorphize: remove predicate logic #75737

davidtwco commented Aug 20, 2020 •

edited

Loading

eddyb commented Aug 23, 2020

This comment has been minimized.

davidtwco commented Oct 16, 2020 •

edited

Loading

This comment has been minimized.

Dylan-DPC-zz commented Nov 24, 2020

davidtwco commented Nov 24, 2020

oli-obk commented Dec 28, 2020

davidtwco commented Jan 13, 2021

bors commented Mar 5, 2021

Dylan-DPC-zz commented Mar 8, 2021

JohnCSimon commented May 17, 2021

camelid commented Sep 2, 2021

davidtwco commented Oct 4, 2021

polymorphize: remove predicate logic #75737

polymorphize: remove predicate logic #75737

Conversation

davidtwco commented Aug 20, 2020 • edited Loading

eddyb commented Aug 23, 2020

This comment has been minimized.

davidtwco commented Oct 16, 2020 • edited Loading

This comment has been minimized.

Dylan-DPC-zz commented Nov 24, 2020

davidtwco commented Nov 24, 2020

oli-obk commented Dec 28, 2020

davidtwco commented Jan 13, 2021

bors commented Mar 5, 2021

Dylan-DPC-zz commented Mar 8, 2021

JohnCSimon commented May 17, 2021

camelid commented Sep 2, 2021

davidtwco commented Oct 4, 2021

davidtwco commented Aug 20, 2020 •

edited

Loading

davidtwco commented Oct 16, 2020 •

edited

Loading