IRC: logic error in the second call to rewrite#774
Conversation
gretay-js
left a comment
There was a problem hiding this comment.
Cfg_irc_state is maintaining an invariant of representation between Reg.irc_work_list and
state.spilled_nodes. It would be better to use State.add_spilled_nodes in run and then rely on rewrite to call State.clear_spilled_nodes.
gretay-js
left a comment
There was a problem hiding this comment.
Looks good, thanks.
If rewrite used State.get_spilled_nodes instead of taking the spilled_nodes as a parameter, we would have avoided the bug. Another improvement is to call State.add_spilled_nodes directly instead of constructing spilling_because_split and spilling_because_split_or_unused lists. Then, the code in main and run that checks the length of the list could perhaps be factored out. This refactoring can be done as a follow-up PR, and probably after PR#794.
|
Discussed off-line, just for the record: hijacking |
The
rewritefunction expects the registers it gets tobe spilled (technically to have their
irc_work_listfield set to
Spilled). Before this pull request, it isnot the case with the second call to
rewrite(theone from
run). I have not been able to producea miscompilation, but I think there is the potential
for one, in particular if the register ends up in the
list because because it is in
resbut not inarg.