Hi!
On Tue, Mar 29, 2022 at 09:52:43AM +0200, Jakub Jelinek wrote:
> The testcase in the PR fails under valgrind on mips64 (but only Martin
> can reproduce, I couldn't).
> But the problem reported there is that SUBST_MODE remembers addresses
> into the regno_reg_rtx array, then some splitter needs a new pseudo
> and calls gen_reg_rtx, which reallocates the regno_reg_rtx array
> and finally undo operation is done and dereferences the old regno_reg_rtx
> entry.
> The rtx values stored in regno_reg_rtx array seems to be created
> by gen_reg_rtx only and since then aren't modified, all we do for it
> is adjusting its fields (e.g. adjust_reg_mode that SUBST_MODE does).
Yes, every pseudo has one and only one mode. If you make sure every use
of the pseudo can have that new mode, this works (as opposed to with
hard registers, where you need more checks).
> So, I think it is useless to use where.r for UNDO_MODE and store
> ®no_reg_rtx[regno] in struct undo, we can store just
> regno_reg_rtx[regno] (i.e. pointer to the REG itself instead of
> pointer to pointer to REG) or could also store just the regno.
That should work yes.
> Or there are variant patches in the PR, either a minimal version of
> this patch, or one that records regno and adjusts all SUBST_MODE uses.
I like this one best I think :-)
> Also, not sure I like very much a function name in all caps, but I wanted
> to preserve the users and all other SUBST and SUBST_* are also all caps.
> Yet another possibility would be keep do_SUBST_MODE with the new
> arguments and
> #define SUBST_MODE(INTO, NEWVAL) do_SUBST_MODE ((INTO), (NEWVAL))
Like the other things in combine.c do, so please change to that? Which
means changing less than you do now :-)
Changing this all to not have macros is better of course, and can be
tastefully done even with C++ (it would use C++ features even :-) ), but
let's do that all at once, and in stage 1.
> - union { rtx *r; int *i; struct insn_link **l; } where;
> + union { rtx *r; int *i; rtx m; struct insn_link **l; } where;
NAK. It is not clear at all what "rtx m" means, esp. since there is an
"rtx *r" already. In the PR you said "machine_mode m", that is clear of
course, can you do that instead?
Okay for trunk with those things fixed. Thanks!
Segher
