Conversation
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.
|
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.) |
|
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. |
|
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? |
|
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? |
|
Oups, thanks for spotting that, sorry this slipped in. Squashing... |
chrysn
force-pushed
the
riscv-vector-types
branch
from
a3f272a to
b2897b8
Compare
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Some small changes I suggested, but generally it looks very good.
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
force-pushed
the
riscv-vector-types
branch
3 times, most recently
from
dfb2b10 to
e6377b2
Compare
|
Has the Darwin pipeline been updated subtly? One failed job shows which is the symptom of #676 (indicating that the image is using LLVM 15 already). |
#677 is not merged yet, so I don't think so. Darwin CI is solely based on |
Rerunning the GitHub actions on the master branch might be helpful here. [edit: They already show that the same failed with the same error] |
|
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? |
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
I think it looks all good now. The Darwin CI failure appears to be totally separate and a coincidence.
3 participants
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
Float16rather than aHalfbuiltin down the road, support for that is added by mapping to Half. That seems to be OK because, unlike C'sintsize mess,halfis commonly understood to mean an IEEE 16-bit float.