Skip to content

Conversation

@IhsaneTahir
Copy link

This PR offers a patch for T-Head openc910 DivSqrt unit instantiated in THMULTI.

It fixes the handling of the underflow exception in the case where the result of the division, as though the exponent range were unbounded, lies strictly between ±b^emin. Even if the rounded result is exactly ±b^emin.

It resolves issue #155

@MikeOpenHWGroup
Copy link
Member

Hi @IhsaneTahir. Typically we do not include patch files to PR as they are redundant once the merge is done. Is there a reason the patch file is needed? If not, please update this PR to remove it. Thanks.

@IhsaneTahir
Copy link
Author

Hi @MikeOpenHWGroup, thanks for pointing this out.

From my understanding, the vendor/openc910 directory is maintained through ./util/vendor.py, so local changes usually need to be captured as patches to avoid being lost when someone runs --update. In that sense, I thought the patch file was needed to keep this fix reproducible across future vendor updates. I also noticed that other bug fixes to this IP in the repo have been handled this way.

That said, if the preferred workflow is to maintain a fork of openc910 and vendor from there instead, I’m happy to adjust and I will remove the file.

@MikeOpenHWGroup
Copy link
Member

Hmmm. That workflow is not documented herein (at least, I didn't find it), and it is not the way most other OpenHW are maintained. Having said that, if that what most downstream users expect, we shouldn't change the flow without notifying everyone. Let me get back to you...

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.

2 participants