Skip to content
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

Implement RustcInternal for StableBody #127782

Closed

Conversation

artemagvanian
Copy link
Contributor

StableMIR has a way to convert internal Body into its stable counterpart, but not the other way around. Being able to perform the conversion both ways could allow using tools like rustc_mir_dataflow, which do not understand stable Body, from StableMIR.

r? celinval

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@celinval celinval Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please move your changes to its own module internal/mir.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled all impls for MIR items into its own file (some of them were already implemented before me)

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 17, 2024

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

@artemagvanian artemagvanian force-pushed the stable-body-to-internal branch from 3322a10 to d08c4f4 Compare July 17, 2024 16:52
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Getting action download info
Download action repository 'msys2/setup-msys2@v2.22.0' (SHA:cc11e9188b693c2b100158c3322424c4cc1dadea)
Download action repository 'actions/checkout@v4' (SHA:692973e3d937129bcbf40652eb9f2f61becf3332)
Download action repository 'actions/upload-artifact@v4' (SHA:0b2256b8c012f0828dc542b3febcab082c67f72b)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt \
    && pip3 install virtualenv
COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/

# NOTE: intentionally uses python2 for x.py so we can test it still works.
# NOTE: intentionally uses python2 for x.py so we can test it still works.
# validate-toolstate only runs in our CI, so it's ok for it to only support python3.
ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test \
           --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---

#12 [5/8] COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
#12 DONE 0.0s

#13 [6/8] RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv
#13 0.427   Downloading binaryornot-0.4.4-py2.py3-none-any.whl (9.0 kB)
#13 0.440 Collecting boolean-py==4.0
#13 0.443   Downloading boolean.py-4.0-py3-none-any.whl (25 kB)
#13 0.455 Collecting chardet==5.1.0
---
#13 3.554 Building wheels for collected packages: reuse
#13 3.555   Building wheel for reuse (pyproject.toml): started
#13 3.895   Building wheel for reuse (pyproject.toml): finished with status 'done'
#13 3.896   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#13 3.896   Stored in directory: /tmp/pip-ephem-wheel-cache-_9lqdj2e/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#13 3.899 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#13 3.923   Attempting uninstall: setuptools
#13 3.923     Found existing installation: setuptools 59.6.0
#13 3.924     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#13 3.924     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
#13 3.925     Can't uninstall 'setuptools'. No files were found to uninstall.
#13 4.622 Successfully installed binaryornot-0.4.4 boolean-py-4.0 chardet-5.1.0 jinja2-3.1.2 license-expression-30.0.0 markupsafe-2.1.1 python-debian-0.1.49 reuse-1.1.0 setuptools-66.0.0
#13 4.622 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#13 5.146 Collecting virtualenv
#13 5.183   Downloading virtualenv-20.26.3-py3-none-any.whl (5.7 MB)
#13 5.278      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 5.7/5.7 MB 60.8 MB/s eta 0:00:00
#13 5.328 Collecting platformdirs<5,>=3.9.1
#13 5.331   Downloading platformdirs-4.2.2-py3-none-any.whl (18 kB)
#13 5.347 Collecting distlib<1,>=0.3.7
#13 5.350   Downloading distlib-0.3.8-py2.py3-none-any.whl (468 kB)
#13 5.390 Collecting filelock<4,>=3.12.2
#13 5.393   Downloading filelock-3.15.4-py3-none-any.whl (16 kB)
#13 5.393   Downloading filelock-3.15.4-py3-none-any.whl (16 kB)
#13 5.482 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#13 5.668 Successfully installed distlib-0.3.8 filelock-3.15.4 platformdirs-4.2.2 virtualenv-20.26.3
#13 DONE 5.8s

#14 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#14 DONE 0.0s
---
DirectMap4k:      194496 kB
DirectMap2M:     6096896 kB
DirectMap1G:    12582912 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/08cdc2fa1a9fd5ba466838f69cfaf062a3a64ad1/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-08cdc2fa1a9fd5ba466838f69cfaf062a3a64ad1-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
fmt check
fmt: checked 5261 files
tidy check
tidy: Skipping binary file check, read-only filesystem
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:572: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:588: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:728: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:742: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:743: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:783: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
##[error]tidy error: /checkout/compiler/rustc_smir/src/rustc_internal/internal/mir.rs:815: TODO is used for tasks that should be done before merging a PR; If you want to leave a message in the codebase use FIXME
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (24.1)
  Downloading pip-24.1.2-py3-none-any.whl.metadata (3.6 kB)
Downloading pip-24.1.2-py3-none-any.whl (1.8 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 1.8/1.8 MB 66.3 MB/s eta 0:00:00
Installing collected packages: pip
---
All checks passed!
checking C++ file formatting
some tidy checks failed

Command LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustc" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build" "4" "--extra-checks=py:lint,cpp:fmt" (failure_mode=DelayFail, stdout_mode=Print, stderr_mode=Print) did not execute successfully.
Created at: src/core/build_steps/tool.rs:1053:23
Executed at: src/core/build_steps/test.rs:1084:29

Build completed unsuccessfully in 0:01:16

@artemagvanian artemagvanian marked this pull request as ready for review July 17, 2024 19:53
@rustbot
Copy link
Collaborator

rustbot commented Jul 17, 2024

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2024

I don't think we should be doing this PR. The existing back-conversions are fairly minimal for the core types like Ty, Const and Lifetime.

Instead things like data flow can be implemented out of tree on top of the stable MIR APIs.

@artemagvanian
Copy link
Contributor Author

Thanks for your feedback, @oli-obk.

We implemented these conversions for our immediate need and wanted to see whether it would make sense to contribute them back to StableMIR.

I agree that implementing data flow analysis infrastructure on top of StableMIR is better, and this is something we would like to do in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants