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

Surround moore.mir.concat with conversion casts to pass values of the type the ops expect #246

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

maerhart
Copy link
Collaborator

This should fix (most of) CI.

Once we add even more operations, we should do some kind of lazy conversion cast insertion instead if eagerly surrounding every op.

Copy link
Owner

@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.

Cool, looks really nice! 👍

@fabianschuiki
Copy link
Owner

That one failing thing in CI is interesting and looks like it is a genuine bug in codegen that wasn't caught with the Rust LLHD codegen before 🙈. Probably some implicit cast missing that makes the mux types line up properly.

@maerhart
Copy link
Collaborator Author

The problem with the remaining testcase is that there is a replication with a multiple of 0 here. I don't know the precise code location where this goes wrong, but it adds a i1 value to the concatenation instead of a i0. Likely one of the frequent std::cmp::max(size, 1) calls when creating a hw::ConstantOp. It'll be hard to fix that because we don't have support for i0 in CIRCT. Did Rust-LLHD have i0 values, I'm not sure anymore?

@fabianschuiki
Copy link
Owner

Huh good question. I think the i0 should actually be fixed in CIRCT by now, and it should be possible to construct such constants. It would be good to track this in a separate issue and not block this PR on it :-)

@maerhart
Copy link
Collaborator Author

I think all the support necessary was added to APInt, but CIRCT doesn't allow it yet. See here.

@maerhart maerhart merged commit cb0053b into fabianschuiki:master Jul 21, 2022
@maerhart maerhart deleted the concat-ucc branch July 21, 2022 10:04
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.

None yet

2 participants