Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
muldiv_c
andmuxadd
peepopts #4740base: main
Are you sure you want to change the base?
Add
muldiv_c
andmuxadd
peepopts #4740Changes from 1 commit
9943b18
9f84a92
a3cc228
ee18e1f
0ddb3d7
2375879
0e15edd
3b73888
8a6c100
cfadf28
b4fa7dc
f9ae66d
2979232
6c5d7cc
5212ad7
8e78720
d81bda8
ff5237f
029c255
7b70ba4
ceb5d2a
0f8356d
50c93f7
53cf38c
d98b224
1c774f4
6db406f
24c012a
32406ea
cd9e45d
1ad1a0a
fc47616
eb4e147
7270cd3
45500f4
bb49acd
325b0e3
e8e806f
b63ef81
b525a02
5b97274
8b08f81
8687e5f
43f4181
72aef58
a1db286
456773e
87b8419
4d984d5
a2bc9b2
4a28e0d
a7fcda9
48a971c
b6d079b
b1930e9
a497be5
7ce0460
095bf92
cd503d4
a731570
e0534a2
39ff442
832c877
b9c3666
11e4446
9450dc1
41a7c72
0b32e09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
as_int()
conversion may overflow if the operands are too wide. We should catch this case and bailThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
b_const_int << offset
can still overflowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to proceed, a test specifically designed for this condition will help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you test
b_const_int.size() + offset <= 31
you should be fineThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err,
b_const.size() + offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
b_const_int_shifted
is shifted up, there's no guarantee it fits intob_const_width
Also this might need special care wrt signedness.
B_SIGNED
on the multiplier may be false butb_const_int_shifted / c_const_int
can be negative due to the divider. I wouldn't be opposed to requiring matching signedness on the divider and multiplier to make the transformation a little easier to reason aboutThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@povik should I change to: mul->setPort(\B, Const(b_const_int_shifted / c_const_int, b_const_width + offset));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work but I suggest constructing the Const with 32 bits (the constructor default) and then calling compress(/signedness/) to fit to the constant. The reason for this is: unless
wreduce
is sequenced afterpeepopt
I am not sure any pass would shrink the operand before we bitblast the multiplierThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented here: akashlevy@8b08f81
and
akashlevy@8687e5f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@povik is this fix acceptable?