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

Bump LLVM to 3cc852ece438a63e7b09d1c84a81d21598454e1a. #7847

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

mikeurbach
Copy link
Contributor

The main difference was in changes related to the APInt constructor.

This was needed when
llvm/llvm-project@3494ee9
landed.

Similar changes were made upstream here:
https://github.com/llvm/llvm-project/pull/80309/files.

Basically, we now expect the width and value arguments to the APInt
constructor to be consistent. There is some implicit extension or
truncation depending on the signedness of the value, and now a flag to
opt into it.

With this change I've simply opted into the flag where necessary,
rather than thinking hard about how to compute the correct width given
the value.

In places where the isSigned flag took the default value of false, I
kept that when I had to explicitly set that flag.

There was exactly one place (CalyxHelpers.cpp), where I actually
flipped the value of isSigned. The comment in the code made me think
this erroneously thought the flag was "isUnsigned".

Copy link
Member

@cgyurgyik cgyurgyik left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikeurbach
Copy link
Contributor Author

Hmm this is now hitting a compile error related to Python extension, which I didn't see locally with Python 3.10.6. Need to see what got pulled in with this bump that could cause issues here...

@mikeurbach
Copy link
Contributor Author

mikeurbach commented Nov 21, 2024

I have pinpointed the error to this change upstream: llvm/llvm-project#115307. I think I can fix it by updating to pybind11 2.11.

@mikeurbach mikeurbach requested a review from teqdruid as a code owner November 21, 2024 23:02
@mikeurbach
Copy link
Contributor Author

mikeurbach commented Nov 22, 2024

Wellp, that fixed the compile time failure, but now two handshake tests are failing...

17: INFO cocotb: assert conv_value == res, f"Expected {res}, got {conv_value}" 
18: INFO cocotb: AssertionError: Expected 0.0, got 0000000000000000000000000000000000000000000000000000000000000000 
19: INFO cocotb: assert 0000000000000000000000000000000000000000000000000000000000000000 == 0.0 

Wondering if this is related to the APInt business... I'm setting up icarus verilog and cocotb to try and reproduce.

This was needed when
llvm/llvm-project@3494ee9
landed.

Similar changes were made upstream here:
https://github.com/llvm/llvm-project/pull/80309/files.

Basically, we now expect the width and value arguments to the APInt
constructor to be consistent. There is some implicit extension or
truncation depending on the signedness of the value, and now a flag to
opt into it.

With this change I've simply opted into the flag where necessary,
rather than thinking hard about how to compute the correct width given
the value.

In places where the `isSigned` flag took the default value of false, I
kept that when I had to explicitly set that flag.

There was exactly one place (CalyxHelpers.cpp), where I actually
flipped the value of `isSigned`. The comment in the code made me think
this erroneously thought the flag was "isUnsigned".
@mikeurbach
Copy link
Contributor Author

I checked the types of the cocotb values in the failing test...

(Pdb) type(conv_value)
<class 'cocotb.binary.BinaryValue'>
(Pdb) type(res)
<class 'float'>

I don't know anything about this stuff, but seems like a great time to use the optional converter argument, since the values looked a whole lot like 0s of different types in the failing comparison, so that's what I did in the last commit.

@teqdruid @mortbopet a couple things...

I'm planning to merge this, but if you have any concerns with the most recent commit please take another look here.

I also saw a problem compiling native Python extensions on the version of pybind11 we had on the integration test docker image, so I've bumped that and updated the requirement in our wheel build. I didn't change anything about PyCDE, but it may need a similar update.

@mikeurbach mikeurbach merged commit 02823af into main Nov 22, 2024
4 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/bump-llvm branch November 22, 2024 03:15
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.

3 participants