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

Mod Arith: migrate barrett/subifge to mod arith type #1138

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bqeyfukqbukq
Copy link

This fix issue #1084

This is wip

This converts barret reduction and subifge to ModArithType, and added corresponding lowering in ModArithToArith.cpp

One thing worth discussing is that the input type is still SignlessIntegerLike, otherwise lowering won't work.

bqeyfukqbukq and others added 5 commits December 4, 2024 11:45
subifge to mod arith

barrett for mul

1

barrett_reduce

barrett_reduce
subifge to mod arith

barrett for mul

1

barrett_reduce

barrett_reduce

barrett reduce test
@j2kun
Copy link
Collaborator

j2kun commented Jan 24, 2025

Hi @bqeyfukqbukq, what is the status of this PR? I think we would like appreciate this contribution, but it has some syntax errors and given its age it likely needs to be rebased and updated.

If you no longer want to work on this, let me know and I can cherry-pick your commits and finish the work.

@ZenithalHourlyRate
Copy link
Collaborator

The author is one of my colleagues in our group and I was trying to introduce the project to him. After a few discussion he decided to do other projects so left the PR here.

I can cherry-pick your commits and finish the work.

One thing that should be handled carefully is the bitwidth of intermediate result in barrett reduction, where the original author used 3B and we (wrongly) used 2B, then realized it should be 3B but did not get a proper type for it.

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