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

Tolerate RISC-V vector types #693

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

chrysn
Copy link
Contributor

@chrysn chrysn commented Sep 27, 2022

Closes: #692

Note that I have no clue how these vector types would be used -- I don't even have a processor that supports them, but their mere presence in LLVM's builtins caused the above-mentioned issue.

This follows along the paths of existing vector types, reusing the code already established for SVE. As this produces a Float16 rather than a Half builtin down the road, support for that is added by mapping to Half. That seems to be OK because, unlike C's int size mess, half is commonly understood to mean an IEEE 16-bit float.

chrysn added a commit to chrysn-pull-requests/riotdocker that referenced this pull request Sep 27, 2022
This uses a branch of the LLVM PR <immunant/c2rust#693>
which fixes C2Rust's issue <immunant/c2rust#692>.
Both have been preexisting for a while, but became critical when
switching over to the current LLVM version.
@chrysn
Copy link
Contributor Author

chrysn commented Sep 27, 2022

The CI failures indicate that the "|| it's one of the riscv vector types" will needs its own ifdef around it.

Before I start digging into LLVM to find the precise version to test for, is the PR in general acceptable? (there's already half a day's work in it, I'd prefer not to add more without a rough thumbs-up-or-thumbs-down.)

@chrysn chrysn changed the title Tolerate RISCV vector types Tolerate RISC-V vector types Sep 27, 2022
@chrysn chrysn mentioned this pull request Sep 27, 2022
@kkysen
Copy link
Contributor

kkysen commented Sep 27, 2022

Thanks for the PR! I think it's generally good. We added similar code in #441 to handle the SVE types, so if it follows that, it should be generally good.

@chrysn
Copy link
Contributor Author

chrysn commented Sep 28, 2022

Now it seems to pass everything that doesn't need extra secrets.

Do you want commits neatly packed into the two it semantically is, or would you squash on merge anyway?

@thedataking
Copy link
Contributor

Given the size of this chang, I think a squash merge is fine – but if you have the time to clean up the commit history, that would be nice too. I noticed you have some commented out code in there – can we get rid of that before merging?

@chrysn
Copy link
Contributor Author

chrysn commented Sep 28, 2022

Oups, thanks for spotting that, sorry this slipped in.

Squashing...

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Some small changes I suggested, but generally it looks very good.

c2rust-ast-exporter/src/AstExporter.cpp Outdated Show resolved Hide resolved
c2rust-ast-exporter/src/AstExporter.cpp Outdated Show resolved Hide resolved
c2rust-ast-exporter/src/AstExporter.cpp Outdated Show resolved Hide resolved
c2rust-ast-exporter/src/AstExporter.cpp Show resolved Hide resolved
@kkysen
Copy link
Contributor

kkysen commented Sep 28, 2022

Do you want commits neatly packed into the two it semantically is, or would you squash on merge anyway?

I think rebasing to 2 semantic commits would be nice, plus possibly this one (#693 (comment)) as a 3rd.

Float16 builtins appear as a consequence of converting RISCV vector
types to normal vector types.

Closes: immunant#692
@chrysn
Copy link
Contributor Author

chrysn commented Sep 28, 2022

Has the Darwin pipeline been updated subtly? One failed job shows

  * Closing connection 1
  /Users/runner/work/1/s/c2rust-ast-exporter/src/AstExporter.cpp:2280:52: error: no member named 'Ascii' in 'clang::StringLiteral::StringKind'
              case clang::StringLiteral::StringKind::Ascii:
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
  1 error generated.

which is the symptom of #676 (indicating that the image is using LLVM 15 already).

@kkysen
Copy link
Contributor

kkysen commented Sep 28, 2022

Has the Darwin pipeline been updated subtly?

#677 is not merged yet, so I don't think so. Darwin CI is solely based on azure-pipelines.yml, unlike the others which are based on Docker images and have to be updated separately from GitHub. azure-pipelines.yml does say it uses macOS-latest, so perhaps that's been updated? @thedataking, thoughts?

@chrysn
Copy link
Contributor Author

chrysn commented Sep 28, 2022

azure-pipelines.yml does say it uses macOS-latest, so perhaps that's been updated?

Rerunning the GitHub actions on the master branch might be helpful here.

[edit: They already show that the same failed with the same error]

@kkysen
Copy link
Contributor

kkysen commented Sep 28, 2022

You're right; I missed that master just started failing.

This is weird: 7e79f91 was the first to fail, starting at 11:06 pm, but then ec2c291 right after succeeded, starting at 11:12 pm.

@kkysen
Copy link
Contributor

kkysen commented Sep 28, 2022

LLVM 15 was indeed used in the failing Darwin CI run. I can't tell what the successful Darwin CI run used, though, as the log on error wasn't printed, but I assume it wasn't LLVM 15. Nothing should be different between those commits, so I'm guessing it's an Azure macOS-latest issue.

@thedataking, help?

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

I think it looks all good now. The Darwin CI failure appears to be totally separate and a coincidence.

@kkysen kkysen merged commit 65076dc into immunant:master Oct 4, 2022
@chrysn chrysn deleted the riscv-vector-types branch November 3, 2022 16:12
@chrysn chrysn restored the riscv-vector-types branch November 4, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RISC-V vector instructions pulled in through LLVM cause build failures
3 participants