Richard Biener <richard.guent...@gmail.com> writes:
> On Thu, Feb 11, 2021 at 4:33 PM Richard Sandiford via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> df_lr_bb_local_compute has:
>>
>> FOR_EACH_INSN_INFO_DEF (def, insn_info)
>> /* If the def is to only part of the reg, it does
>> not kill the other defs that reach here. */
>> if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
>>
>> However, as noted in the comment in the patch and below, almost all
>> partial definitions have an associated use. This means that the
>> confluence function:
>>
>> IN = (OUT & ~DEF) | USE
>>
>> is unaffected by whether partial definitions are in DEF or not.
>>
>> Even though the choice doesn't matter for the LR problem itself,
>> it's IMO much more convenient for consumers if DEF contains all the
>> definitions in the block. The only pre-RTL-SSA code that tries to
>> consume DEF directly is shrink-wrap.c, which already has to work
>> around the incompleteness of the information:
>>
>> /* DF_LR_BB_INFO (bb)->def does not comprise the DF_REF_PARTIAL and
>> DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e.
>> at -O1, just give up searching NEXT_BLOCK. */
>>
>> I hit the same problem when trying to fix the RTL-SSA part of PR98863.
>>
>> This patch treats partial definitions as both a def and a use,
>> just like the df_ref records almost always do.
>>
>> To show that partial definitions almost always have uses:
>>
>> DF_REF_CONDITIONAL:
>>
>> Added by:
>>
>> case COND_EXEC:
>> df_defs_record (collection_rec, COND_EXEC_CODE (x),
>> bb, insn_info, DF_REF_CONDITIONAL);
>> break;
>>
>> Later, df_get_conditional_uses creates uses for all DF_REF_CONDITIONAL
>> definitions.
>>
>> DF_REF_PARTIAL:
>>
>> In total, there are 4 locations at which we add partial definitions.
>>
>> Case 1:
>>
>> if (GET_CODE (dst) == STRICT_LOW_PART)
>> {
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL |
>> DF_REF_STRICT_LOW_PART;
>>
>> loc = &XEXP (dst, 0);
>> dst = *loc;
>> }
>>
>> Corresponding use:
>>
>> case STRICT_LOW_PART:
>> {
>> rtx *temp = &XEXP (dst, 0);
>> /* A strict_low_part uses the whole REG and not just the
>> SUBREG. */
>> dst = XEXP (dst, 0);
>> df_uses_record (collection_rec,
>> (GET_CODE (dst) == SUBREG) ? &SUBREG_REG (dst) :
>> temp,
>> DF_REF_REG_USE, bb, insn_info,
>> DF_REF_READ_WRITE | DF_REF_STRICT_LOW_PART);
>> }
>> break;
>>
>> Case 2:
>>
>> if (GET_CODE (dst) == ZERO_EXTRACT)
>> {
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL | DF_REF_ZERO_EXTRACT;
>>
>> loc = &XEXP (dst, 0);
>> dst = *loc;
>> }
>>
>> Corresponding use:
>>
>> case ZERO_EXTRACT:
>> {
>> df_uses_record (collection_rec, &XEXP (dst, 1),
>> DF_REF_REG_USE, bb, insn_info, flags);
>> df_uses_record (collection_rec, &XEXP (dst, 2),
>> DF_REF_REG_USE, bb, insn_info, flags);
>> if (GET_CODE (XEXP (dst,0)) == MEM)
>> df_uses_record (collection_rec, &XEXP (dst, 0),
>> DF_REF_REG_USE, bb, insn_info,
>> flags);
>> else
>> df_uses_record (collection_rec, &XEXP (dst, 0),
>> DF_REF_REG_USE, bb, insn_info,
>> DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT);
>> ----------------------------^^^^^^^^^^^^^^^^^
>> }
>> break;
>>
>> Case 3:
>>
>> else if (GET_CODE (dst) == SUBREG && REG_P (SUBREG_REG (dst)))
>> {
>> if (read_modify_subreg_p (dst))
>> flags |= DF_REF_READ_WRITE | DF_REF_PARTIAL;
>>
>> flags |= DF_REF_SUBREG;
>>
>> df_ref_record (DF_REF_REGULAR, collection_rec,
>> dst, loc, bb, insn_info, DF_REF_REG_DEF, flags);
>> }
>>
>> Corresponding use:
>>
>> case SUBREG:
>> if (read_modify_subreg_p (dst))
>> {
>> df_uses_record (collection_rec, &SUBREG_REG (dst),
>> DF_REF_REG_USE, bb, insn_info,
>> flags | DF_REF_READ_WRITE | DF_REF_SUBREG);
>> break;
>> }
>>
>> Case 4:
>>
>> /* If this is a multiword hardreg, we create some extra
>> datastructures that will enable us to easily build REG_DEAD
>> and REG_UNUSED notes. */
>> if (collection_rec
>> && (endregno != regno + 1) && insn_info)
>> {
>> /* Sets to a subreg of a multiword register are partial.
>> Sets to a non-subreg of a multiword register are not. */
>> if (GET_CODE (reg) == SUBREG)
>> ref_flags |= DF_REF_PARTIAL;
>> ref_flags |= DF_REF_MW_HARDREG;
>>
>> Corresponding use:
>>
>> None. However, this case should be rare to non-existent on most
>> targets, and the current handling seems suspect. See the comment
>> in the patch for more details.
>>
>> Tested so far on aarch64-linux-gnu, both individually and with
>> the follow-on rtl-ssa patch. WDYT? I realise it's not ideal
>> to be applying this in stage 4, but it would be expensive
>> to work around.
>>
>> FWIW, I also tried a variation:
>>
>> df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
>> FOR_EACH_INSN_INFO_DEF (def, insn_info)
>> {
>> unsigned int dregno = DF_REF_REGNO (def);
>> bitmap_set_bit (&bb_info->def, dregno);
>> bitmap_clear_bit (&bb_info->use, dregno);
>> }
>>
>> FOR_EACH_INSN_INFO_USE (use, insn_info)
>> /* Add use to set of uses in this BB. */
>> bitmap_set_bit (&bb_info->use, DF_REF_REGNO (use));
>>
>> if (CHECKING_P)
>> FOR_EACH_INSN_INFO_DEF (def, insn_info)
>> if (DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL))
>> gcc_assert (bitmap_bit_p (&bb_info->use, DF_REF_REGNO (def)));
>>
>> This obviously triggers case 4 above, as seen in the testcase.
>> It nevertheless survived bootstrap & regression test. This is what
>> I expected, because case 4 should never trigger on most targets,
>> including AArch64.
>>
>> I think this shows that the LR in/out sets are very likely to be
>> unaffected by the patch.
>
> I'd say it should nevertheless be a correctness fix. I suppose you
> ran into the cases where it was _not_ unaffected by the patch?
> (the multi-hard-reg subreg case?)
The problems I ran into without the patch were from cases (1)-(3).
E.g. I was trying to place phis using a combination of the def sets
and dominance frontiers, but strict_lowpart definitions weren't in
the def sets, and so we were missing necessary phis.
I then wanted to go with the asserting variant above, but when
I looked at the df-scan.c code, it was obvious that case (4) would
trigger the assert.
I don't know a way of triggering case (4) for real code (for any
target). The only way I could think of was to use an artificial
rtl test like the one in the patch. That did trigger the assert
above, but compiles correctly with the patch as posted (and compiled
correctly before the patch).
So…
> The comment mentions that the bug might be elsewhere though (in
> df-scan I suppose), so is isn't it better to fix the missing USE or
> making it two KILLs there? I realize the fallout of that might be
> bigger at this point.
…yeah, I agree the right long-term fix would be to remove:
/* Sets to a subreg of a multiword register are partial.
Sets to a non-subreg of a multiword register are not. */
if (GET_CODE (reg) == SUBREG)
ref_flags |= DF_REF_PARTIAL;
from case 4 and see if anything breaks. But that feels quite risky
at this stage, given that it's so hard to trigger.
I don't know of a way of constructing a test that is actually
miscompiled because of the current handling of case (4). The rtl test
was compiled correctly, it just had surprising LR information before
the patch.
That said, if you think it's worth trying now, I'd be happy to remove
the code above and go for the asserting version.
Thanks,
Richard
