On 03/10/2016 11:10 AM, Bernd Schmidt wrote:
When I submitted my previous lra-remat patch, I mentioned I had some
concerns about the way we dealt with register number comparisons, but I
didn't want to change things blindly without a testcase. PR70123 has now
provided such a testcase where we are trying to rematerialize a hard
register (r6). While scanning we encounter an instruction of the form
(set (reg 285) (reg 272))
i.e. involving only pseudos, but reg_renumber[285] is r6. Since we only
compare register numbers, we do not notice that the hard reg is clobbered.
The following patch modifies the function input_regno_present_p, and
also renames it so that its purpose is more obvious to someone familiar
with other parts of gcc. I've made it look at reg_renumber, and also try
to deal with multi-word hard registers properly.
I'm not entirely sure this is a fully safe approach however, since I
can't yet answer the question of whether LRA could change another pseudo
to reside in hard register 6, thereby making the rematerialization
invalid after the fact. Therefore the patch also includes a change to
just disable candidates if they involve hard registers. I haven't
observed that making any difference in code generation (on x86_64),
beyond fixing the testcase on s390.
Bootstrapped and tested on x86_64-linux; Jakub verified that the
testcase works afterwards. Ok for trunk and 5-branch, either for one or
for both parts? I'm hoping the testcase in gcc.dg/torture will get
exercised in the right way on s390, but I haven't run tests on that
machine.
Bernd
remat-hardregs.diff
PR target/70123
* lra-remat.c (operand_to_remat): Disallow hard regs in the value t
be rematerialized.
(reg_overlap_for_remat_p): Renamed from input_regno_present_p.
Arguments swapped. All callers changed. Take reg_renumber into
account, and Calculate and compare register ranges for hard regs.
PR target/70123
* gcc.dg/torture/pr70123.c: New test.
OK. Like you I'm not sure if the operand_to_remat test is strictly
necessary, but I can't see how it'll ever generate incorrect code.
The change to reg_overlap_for_remat_p looks good and should be strictly
an improvement from a correctness standpoint.
I think both are OK for the trunk and the gcc-5 branch after a bit of
soaking on the trunk.
jeff
