Fix ir_utils._max_label being updated incorrectly
#7156
Conversation
It is now changed to an object to avoid mutating global variable. The `_MaxLabel` class also enforce that its value must be mutated via the update method.
|
The tests are taking overly long because of high memory consumption. The problem is not unique to this PR branch. On mainline, running |
Like this: #7028 (comment) ? |
| self._value = max(newval, self._value) | ||
|
|
||
|
|
||
| _the_max_label = _MaxLabel() |
There was a problem hiding this comment.
Now delete the class? There should only be one of these so make it awkward to obtain another?
| ir_utils._max_label = max_label | ||
| ir_utils._the_max_label.update(max_label) |
There was a problem hiding this comment.
One of the bugs. I stopped looking further for where exactly the problems are and just batch fixes all the _max_label
| f_ir.blocks = add_offset_to_labels(f_ir.blocks, _max_label + 1) | ||
| f_ir.blocks = add_offset_to_labels(f_ir.blocks, _the_max_label.next()) |
There was a problem hiding this comment.
This is different? It's advancing the counter whereas originally the offset was set as _max_label+1 but the counter was not updated?
There was a problem hiding this comment.
The counter not updated part had me worried. That's why i changed it to advance. Worse case is that there's a hole the label space, but I want to be certain that there is no reuse of label.
There was a problem hiding this comment.
I agree, it seems like it ought to update else there is potential for reuse, and like you said, worse case is that the label space has gaps which is relatively harmless.
| target_registry["CustomCPU"] = CustomCPU | ||
| dispatcher_registry[target_registry["CustomCPU"]] = CustomCPUDispatcher |
There was a problem hiding this comment.
This is ok because this test is run in a subprocess. Were it run in the test suite there might be a name collision for the string "CustomCPU" had another test registered that as a target.
There was a problem hiding this comment.
Right. The test is also highly dependent of the internal state of ir_utils that it must be executing in a fresh process to replicate.
stuartarchibald
added
4 - Waiting on reviewer
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
stuartarchibald
added
4 - Waiting on CI
|
cancelling CI build until we have #7160 merged |
sklam
added
5 - Ready to merge
|
CI passed. Merging |
Fixes #7155.
This patch changes
ir_utils._max_labelinto an instance of_MaxLabelso it is easier to enforce the correct update logic. It also means that we no longer need to useglobal _max_labelto update it.Additional notes
As demonstrate by this patch, #7155 is not caused by the retargeting support or any of the recent work to make target-specific overload. It is just unfortunate that the bug is revealed in a case that make use of the retargeting support. I'm also unsure if we really should keep the included testcase, which is not representative of the problem but just a coincident that it revealed the problem. It is likely overly difficult to create a minimal reproducer for the problem.