-
Notifications
You must be signed in to change notification settings - Fork 866
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
Extend the peepopt
shiftmul matcher
#3873
Conversation
Force pushed. There was a confusion in the commit message about whether it's the left or right shift matcher we are adding. |
As we already started to discuss in #3833, this currently only handles: Since yesterday I went through different scenarios/cases I could see happening and thought about how to handle them.
Tl;Dr: assuming |
Not really,
Yeah, I am not sure how often this would come up in actual code but it's not inconceivable. Maybe what other RTL compilers do in such a case would be of useful input since existing codebases could be accustomed to that. In any case I guess there should be some heuristics comparing the magnitude of |
You are correct in both cases (I already noticed the first one myself). Other compilers are unlikely to use a concept like |
I am not sure there's an issue there, and if there is overhead, maybe the situation can be improved by only leaving those signals connected to the shift cell that can actually be selected, and rounding up |
Well but then at some point if |
Makes sense, I mentally didn't make |
Personally I think there is a pretty strong case for transforming it to a A block-mux halves the bits it works on in every stage, so its cost should be something like Anyway, one would expect a block-mux to be more efficient in most cases, especially the ones you are likely to encounter in reality. Obviously and arbitrarily good MUX-tree optimizer will likely achieve identical results but the amount of cells you have to operate on matters, especially once you try to synthesize larger designs. |
I don't think that's true since if each stage works on a different amount of bits, and that amount doubles going to earlier stages, the final cost must come down to something like
Agreed on shift having a lower cost (in terms of elementary muxes) for the case
I agree. Not so much because of any overhead consideration but for clarity and to make it easier for later passes to reason about what's going on in the circuit. |
By the way I am currently working on a shiftadd.pmg that should be able to deal with any The more common Currently I just limited it so that one value must be constant ( |
Yes you are correct, it even says that on the paper in front of me, I just didn't pay attention while typing it (well I didn't have the -1 but I ignored it on purpose since its just a constant). |
Good to know!
Yeah, I don't see any other reasonable way to do it.
I think it's best if you open a separate PR (a draft one if you don't feel it's ready for merging), and then we can discuss the shiftadd rule there. I don't have merging powers here but those that have will be happy we split things. |
I will be opening a new PR, in the last user-meeting they said they prefer more small PRs since they are easier to review, exactly what you are saying as well. Will you try to use I guess the second approach is probably cleaner for now, the other one should probably also be added as a separate PR. |
Yeah, I think I will make the
Agreed as a prospect if what you mean by |
Err, I wrote something bit confusing earlier...
I was thinking the untransformed shifter has a lower cost than the block-mux, but that is without accounting for the cost of the |
So as I see it we have three options we need to decide between when stumbling on a shiftmul pattern: Transform to |
Your naming scheme is fine as well, I just think its important to keep the original (ie
Yes exactly, except that we don't really decide. Each option is just a separate |
filter !port(shift, \B).empty() | ||
endmatch | ||
|
||
match neg |
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.
match neg | |
match neg | |
select neg->type == $neg |
Without this it tries to access port \Y
on any cell, no matter the type, this gives an error as not all cells have a \Y
port.
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.
Ha! Of course
} else { | ||
// case of `$shl` | ||
shift_amount = port(shift, \B); | ||
if (!param(neg, \B_SIGNED).as_bool()) |
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.
if (!param(neg, \B_SIGNED).as_bool()) | |
if (!param(shift, \B_SIGNED).as_bool()) |
Likely a typo. If neg
doesn't exist we also can't access any parameters.
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.
Good catch
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.
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.
Yes, can add that to the commit message
passes/pmgen/peepopt.cc
Outdated
if (genmode == "shiftmul") | ||
GENERATE_PATTERN(peepopt_pm, shiftmul); | ||
GENERATE_PATTERN(peepopt_pm, shiftmul_right); |
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.
GENERATE_PATTERN(peepopt_pm, shiftmul_right); | |
log_error("Abort in %s:%d: peepopt-shiftmul does not implement a generate option\n", __FILE__, __LINE__) |
Doesn't this require a generate block in the pattern to actually work?
Running it ends with:
ERROR: pmgen generator is stuck: 10000 iterations with no matching module generated.
I think the most relevant PR is #1298.
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 don't know, I never looked into the generate stuff. Then maybe we should remove the generate flag altogether, I don't think muldiv has any generate blocks either.
The two actual code changes should make it pass the current tests. Personally I also think we should add |
Thanks. I think we should mainly expand the help message, not sure if the patterns each warrant their |
yeah that makes more sense, help it is! |
Drop code that was once used by the 'dffmux' pattern but now is unused after that pattern has been obsoleted by the 'opt_dff' pass.
No functional change intended.
The `opt_expr` pass running before `peepopt` can interfere with the detection of a shiftmul pattern due to some of the bottom bits of the shift amount being replaced with constant zero. Extend the detection to cover those situations as well.
Add a separate shiftmul pattern to match on left shifts which implement demuxing. This mirrors the right shift pattern matcher but is probably best kept separate instead of merging the two into a single matcher. In any case the diff of the two matchers should be easily readable.
Rebased, squashed the fixup into its parent commit, last three commits are new. |
This extends the shiftmul matcher in two useful ways:
opt_expr
ran before and appliedopt.opt_expr.mul_low_zeros
.Related to #3833.