Fixes to bounds inference on shift_left #5477
Conversation
… allow overflow (failes correctness/simplify test)
|
Note: when these changes are merged, they should be squashed. Some of the intermediate commits have buggy code as I had some commits that relied on shift behaviors that are not currently expected behavior (dealing with overflow). |
| @@ -1235,8 +1235,26 @@ class Bounds : public IRVisitor { | |||
| !b_interval.min.type().is_uint() && | |||
There was a problem hiding this comment.
Is it the case that b_interval.min and b_interval.max are always the same type? (i.e., this implies that b_interval.max is also a non-uint?)
There was a problem hiding this comment.
I believe that this should be the case, but @abadams might contradict me. I have been operating under the assumption that that invariant is maintained.
There was a problem hiding this comment.
(Would be nice to have that documented somewhere, though that's orthogonal to this PR.)
I ask mainly because code lower down seemed to be optimized based on that assumption, but it wasn't clear to me whether deliberate or not.
There was a problem hiding this comment.
I agree, all of the changes I have made to bounds inference assumes that the type of the min, type of the max, and type of the operation are all equal.
|
@steven-johnson Any idea what the test failures are caused by? Edit: this question might be null-ed out because I need to add a change. Let's see. |
|
What's the work remaining on this PR? |
|
None, it can be merged if you and @abadams are happy with it. Sorry for the delay! |
|
The failures all appear unrelated, good to merge I think |
One of the shift_left rules (for the min of an interval) was incorrect, but can only be replaced by weaker rules. All of the added rules have been formally verified.
@abadams @dsharlet
Edit: I closed the original draft for this PR (#5476) to get the buildbots to run tests