Support for [lsb+:width] slices #4917
Conversation
ChrisDodd
force-pushed
the
cdodd-slice
branch
2 times, most recently
from
4a72192
to
422673f
Compare
fruffy
added
the
p4-spec
ChrisDodd
force-pushed
the
cdodd-slice
branch
5 times, most recently
from
379c843
to
7ad797d
Compare
- allows for non-const lsb slices (width must still be const) Signed-off-by: Chris Dodd <[email protected]>
ChrisDodd
force-pushed
the
cdodd-slice
branch
from
7ad797d
to
b7579c6
Compare
| static Expression make(Expression a, unsigned hi, unsigned lo); | ||
| } | ||
|
|
||
| class AbsSlice : Slice { |
There was a problem hiding this comment.
The name AbsSlice is a little unclear. I would write it out. Or change it to "ConstantSlice"?
There was a problem hiding this comment.
Abs here is short for Absolute, which seems a better, more descriptive name than Constant to me. I could make it AbsoluteSlice intead, but that just seems excessively wordy. Another possibility is something like FixedSlice. However, none of these names seem 100% correct as "this is obvious what it means"
There was a problem hiding this comment.
Hmm, I have begun to prefer explicit naming over abbreviations because it reduces the cognitive load slightly when dealing with a new code base.
So it looks like they are called Variable Slices in SystemVerilog. How about naming the base class AbstractSlice, Slice, VariableSlice? Or AbstractSlice Constant/Fixed/AbsoluteSlice VariableSlice.
There was a problem hiding this comment.
AbstractSlice, Slice, VariableSlice
I'd lean towards this naming if possible
There was a problem hiding this comment.
I was avoiding renaming the abstract base class (Slice) so as to minimize changes to other parts of the code/backends that don't really care about the details of what kind of slice it is. I also used PlusSlice in case we also wanted to add MinusSlice for [msb-:width] slices in the future.
| Slice(Util::SourceInfo si, Expression a, int hi, int lo) | ||
| : Operation_Ternary(si, IR::Type::Bits::get(hi-lo+1), a, new Constant(hi), new Constant(lo)) {} | ||
| Slice { | ||
| unsigned getH() const override { return e1->to<IR::Constant>()->asUnsigned(); } |
| : Operation_Ternary(si, IR::Type::Bits::get(hi-lo+1), a, new Constant(hi), new Constant(lo)) {} | ||
| Slice { | ||
| unsigned getH() const override { return e1->to<IR::Constant>()->asUnsigned(); } | ||
| unsigned getL() const override { return e2->to<IR::Constant>()->asUnsigned(); } |
There was a problem hiding this comment.
These weren't checked casts before, so I was just keeping the same behavior.
There was a problem hiding this comment.
Yes, but this seems to lead a nullpointer access if the cast does not work.
There was a problem hiding this comment.
Which has the same net result -- an internal compiler error.
There was a problem hiding this comment.
True, but checkedTo adds some more output to the error.
Initial support for Verilog-style
[lsb+:width]slices, allowing variable index slices with a constant width.Most backends will probably require that these be strength-reduced to constant (normal) indexes, which will happen with eg. unrolled loops where the index depends on the loop index.