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.

Sign up for GitHub

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

Fix expansion of signed ranges to masks #3212

Merged
merged 11 commits into from
Apr 20, 2022
Merged

Fix expansion of signed ranges to masks #3212

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8fa2514
Add compilation tests for signed ranges
vlstill Apr 11, 2022
be08558
Implement support for signed ranges
vlstill Apr 11, 2022
34a3ffb
Add gtests for signed range rewriting size
vlstill Apr 11, 2022
eff90ee
Add results for signed range tests
vlstill Apr 11, 2022
5681bfc
Add one more test for signed ranges
vlstill Apr 14, 2022
80f68b9
Remove extraneous debug prints from signed range gtests
vlstill Apr 14, 2022
9444809
Refactor select range replacement a bit
vlstill Apr 14, 2022
a2c8f65
Add more gtests for signed ranges
vlstill Apr 14, 2022
1a9a2d9
Use size_t for indices
vlstill Apr 19, 2022
a62a610
Update a comment
vlstill Apr 19, 2022
aad48f4
Avoid the inSelect flag
vlstill Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactor select range replacement a bit
  • Loading branch information
@vlstill
vlstill committed Apr 14, 2022
commit 9444809ac98a353e18268aab58bc3dc76e733580
99 changes: 49 additions & 50 deletions midend/replaceSelectRange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,51 @@

namespace P4 {

// expands subranges that do not cross over zero
static void
expandRange(const IR::Range *r, std::vector<const IR::Mask *> &masks, const IR::Type *maskType,
big_int min, big_int max)
{
int width = r->type->width_bits();
BUG_CHECK(width > 0, "zero-width range is not allowed %1%", r->type);
big_int size_mask = ((((big_int) 1) << width) - 1);
auto base = r->left->to<IR::Constant>()->base;

BUG_CHECK((min >= 0) == (max >= 0),
"Wrong subrange %1%..%2% (going over zero)", min, max);
if (min < 0) {
// convert negative range to bit-corresponding positive range
min = size_mask + min + 1;
max = size_mask + max + 1;
}

BUG_CHECK(min <= max, "range bounds inverted %1%..%2%", min, max);

big_int range_size_remaining = max - min + 1;

while (range_size_remaining > 0) {
// this generates two kinds of mask entries
// - 0..2^N - 1 where N is the largest number such that this does not
// overshoot max -- to cover all numbers lower then 2^N - 1
// with one mask entry.
// - M..M+2^N - 1 to cover remaining entries with masks that fix a bit
// prefix and leave the last N bits arbitrary.
big_int match_stride = ((big_int) 1) << ((min == 0) ? floor_log2(max + 1) : ffs(min));

while (match_stride > range_size_remaining)
match_stride >>= 1;

big_int mask = ~(match_stride - 1) & size_mask;

auto valConst = new IR::Constant(maskType, min, base, true);
auto maskConst = new IR::Constant(maskType, mask, base, true);
masks.push_back(new IR::Mask(r->srcInfo, valConst, maskConst));

range_size_remaining -= match_stride;
min += match_stride;
}
}

std::vector<const IR::Mask *>
DoReplaceSelectRange::rangeToMasks(const IR::Range *r, int keyIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's actually nicer to return a list of vectors for the result.
one in the common case, two if the range contains 0.
the keyIndex and signedIndicesToReplace make the data flow much harder to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that signedIndicesToReplace does make the flow harder to understand. But this is mainly because it is indeed complex -- the indices are detected when range is expanded, but they are used in the corresponding select through which we backtrack later (making use of postorder). This needs to be done this way as at the time we encounter the select first, we don't know which values will contain ranges and when working with ranges we cannot replace a parent node (at least I don't think so, I could be wrong since I am a beginner to p4c). The only alternative to indices I see would be to remember the actual corresponding select expressions, but that in my opinion improves nothing and could lead to danger in case of some weird select with duplicated keys:

select (foo.a, foo.b, foo.a) {
   -5..5, 42, _ : ...
   _, 16, -32000 : ...
}

In this (rather pathological) case, we need to insert the bitcast only to the first occurence of foo.a not to both instances of foo.a expression in select.

As for the vector-of-vectors. I don't think this is a good idea as returning a vector of vectors implies that there can be any number of vectors, including 0 and more than 2. Therefore I think that would be bad design of the function as the type would allow too many invalid possibilities. It could make sense to return pair (isSigned, vectors) and resolve the index one level up in the postorder of mask.

For now, I have just added a comment about the indices into the class.

std::vector<const IR::Mask *> masks;
Expand Down Expand Up @@ -52,61 +97,15 @@ DoReplaceSelectRange::rangeToMasks(const IR::Range *r, int keyIndex) {
bool isSigned = inType->isSigned;
auto maskType = isSigned ? new IR::Type_Bits(inType->srcInfo, inType->size, false) : inType;

auto base = l->base;
int width = r->type->width_bits();
BUG_CHECK(width > 0, "zero-width range is not allowed %1%", r->type);
big_int size_mask = ((((big_int) 1) << width) - 1);

if (isSigned) {
signedIndicesToReplace.emplace(keyIndex);
}

std::vector<std::pair<big_int, big_int>> subranges;
if (isSigned && left < 0 && right > 0) {
subranges.emplace_back(left, (big_int)-1);
subranges.emplace_back((big_int)0, right);
if (isSigned && left < 0 && right >= 0) {
expandRange(r, masks, maskType, left, (big_int)-1);
expandRange(r, masks, maskType, (big_int)0, right);
} else {
subranges.emplace_back(left, right);
}

for (auto sub : subranges) {
BUG_CHECK((sub.first >= 0) == (sub.second >= 0),
"Wrong subrange %1%..%2%", sub.first, sub.second);
big_int min;
big_int max;
if (sub.first >= 0) {
std::tie(min, max) = sub;
} else {
// convert negative range to bit-corresponding positive range
min = size_mask + sub.first + 1;
max = size_mask + sub.second + 1;
}

BUG_CHECK(min <= max, "range bounds inverted %1%..%2%", min, max);

big_int range_size_remaining = max - min + 1;

while (range_size_remaining > 0) {
// this generates two kinds of mask entries
// - 0..2^N - 1 where N is the largest number such that this does not
// overshoot max -- to cover all numbers lower then 2^N - 1
// with one mask entry.
// - M..M+2^N - 1 to cover remaining entries with masks that fix a bit
// prefix and leave the last N bits arbitrary.
big_int match_stride = ((big_int) 1) << ((min == 0) ? floor_log2(max + 1) : ffs(min));

while (match_stride > range_size_remaining)
match_stride >>= 1;

big_int mask = ~(match_stride - 1) & size_mask;

auto valConst = new IR::Constant(maskType, min, base, true);
auto maskConst = new IR::Constant(maskType, mask, base, true);
masks.push_back(new IR::Mask(r->srcInfo, valConst, maskConst));

range_size_remaining -= match_stride;
min += match_stride;
}
expandRange(r, masks, maskType, left, right);
}

return masks;
Expand Down
3 changes: 3 additions & 0 deletions midend/replaceSelectRange.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ class DoReplaceSelectRange : public Transform {
// Each case is a key set expression.
const uint MAX_CASES;
bool inSelect = false;
// Collects select indices which will need to be replaced with bitcast of
// the original value to unsigned. This is needed if we encounter a range
// over a signed value at the given index.
std::set<int> signedIndicesToReplace;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a set of size_t or unsigned values?
I would change this comment to say "an index i is in this set if selectExpression->components[i] needs to be cast from int to bit". This is only needed if there is a label that has in the i-th position a range expression that contains 0 inside the range".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Because old habits die hard.) Changed to size_t.

As for the comment, sure, except that we need to replace it any time the value is signed, not only when it crosses over zero. Masks are only defined for bit values in P4. I changed it.


explicit DoReplaceSelectRange(uint max) : MAX_CASES(max) {
Expand Down