This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix postreload liveness computation
Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> This causes a miscompilation of eh_personality.cc on sh-elf with a patch I'm
>> working on.
>
> Out of curiosity, which part and how?
I think it was read_encoded_value_with_base, but don't quote me on that.
>> The CFG itself is valid at this point, so I think the fix is simply
>> to compute the union of all successor non-fallthru live_at_start sets.
>
> Sounds OK to me, but the implementation looks a litte convoluted. :-)
>
>> + /* Set up BB_LIVE_AT_START[B] to the set of hard registers live at
>> + the beginning of block B. This information is only valid for
>> + the entry and exit blocks, or for those that start with labels.
>> + The live-at-start sets are no longer valid for blocks that are
>> + only reached via fallthrough; reload may have inherited from the
>> + previous block. */
>> + bb_live_at_start = XNEWVEC (HARD_REG_SET, last_basic_block);
>> + FOR_ALL_BB (bb)
>> + {
>> + live = &bb_live_at_start[bb->index];
>> + REG_SET_TO_HARD_REG_SET (*live, bb->il.rtl->global_live_at_start);
>> + compute_use_by_pseudos (live, bb->il.rtl->global_live_at_start);
>> }
>
> What about skipping the computation when it is not valid to match the comment?
>
> if (EDGE_COUNT (bb->preds) == 1
> && (EDGE_PRED ((bb), 0)->flags & EDGE_FALLTHRU) != 0)
OK.
>> @@ -778,6 +766,35 @@ reload_combine (void)
>> if (! INSN_P (insn))
>> continue;
>>
>> + /* See if INSN is the end of a basic block. */
>> + new_bb = BLOCK_FOR_INSN (insn);
>> + if (new_bb && new_bb != bb)
>> + {
>> + bb = new_bb;
>
> Why not use BB_END?
You mean:
new_bb = BLOCK_FOR_INSN (insn);
if (new_bb && BB_END (new_bb) == insn)
? I thought the version above would be a little more efficient.
There's no need to go derefencing BB_END (...) for every insn -- and
potentially pulling it into cache, since most insns don't otherwise
care about *BB -- when we're entering every basic block from the end
anyway. And in context, it seems just as natural to me.
>> + all_live = NULL;
>> + FOR_EACH_EDGE (e, ei, new_bb->succs)
>> + if ((e->flags & EDGE_FALLTHRU) == 0)
>> + {
>> + live = &bb_live_at_start[e->dest->index];
>> + if (!all_live)
>> + all_live = live;
>> + else
>> + {
>> + if (all_live != &all_live_tmp)
>> + {
>> + COPY_HARD_REG_SET (all_live_tmp, *all_live);
>> + all_live = &all_live_tmp;
>> + }
>> + IOR_HARD_REG_SET (all_live_tmp, *live);
>> + }
>> + }
>> +
>> + if (all_live)
>> + for (i = FIRST_PSEUDO_REGISTER - 1; i >= 0; --i)
>> + if (TEST_HARD_REG_BIT (*all_live, i))
>> + reg_state[i].use_index = -1;
>> + }
>
> Hum... so now that we have a nice HARD_REG_SET API, we don't use it?
Another attempt to be efficient, this time on targets with large
register sets. The common case is that there is at most one
non-fallthrough edge, and it seemed silly to clear, ior into and then
test-for-empty (for example) a 188-bit set on MIPS in that common case.
A more natural way of doing this would IMO have been to have an inner
loop iterate over second and subsequent non-fallthrough edges, but I
don't think that's easy to do with the FOR_EACH_EDGE construct.
OTOH, if you think that's unwarranted micro-optimisation, I can do it...
> Why not the straightforward following?
>
> CLEAR_HARD_REG_SET (...)
>
> FOR_EACH_EDGE (e, ei, new_bb->succs)
> if ((e->flags & EDGE_FALLTHRU) == 0)
> IOR_HARD_REG_SET (...)
>
> if (!hard_reg_set_empty_p (...))
> for (i = FIRST_PSEUDO_REGISTER - 1; i >= 0; --i)
> if (TEST_HARD_REG_BIT (..., i))
> ...
...this way instead.
Richard