Hi Nick,
Thanks for the comments.
Nicholas Krause <xerofo...@gmail.com> writes:
>> Index: gcc/passes.def
>> ===================================================================
>> --- gcc/passes.def 2019-10-29 08:29:03.224443133 +0000
>> +++ gcc/passes.def 2019-11-17 23:15:31.200500531 +0000
>> @@ -437,7 +437,9 @@ along with GCC; see the file COPYING3.
>> NEXT_PASS (pass_inc_dec);
>> NEXT_PASS (pass_initialize_regs);
>> NEXT_PASS (pass_ud_rtl_dce);
>> + NEXT_PASS (pass_combine2_before);
>> NEXT_PASS (pass_combine);
>> + NEXT_PASS (pass_combine2_after);
>> NEXT_PASS (pass_if_after_combine);
>> NEXT_PASS (pass_jump_after_combine);
>> NEXT_PASS (pass_partition_blocks);
>> Index: gcc/timevar.def
> This is really two passes it seems or at least functions. Just a nit but you
> may want to state that as I don't recall reading that.
It's really two instances of the same pass, but yeah, each instance
goes under a different name. This is because each instance needs to
know which bit of the run-combine value it should be testing:
>> The patch adds two instances of the new pass: one before combine and
>> one after it. By default both are disabled, but this can be changed
>> using the new 3-bit run-combine param, where:
>>
>> - bit 0 selects the new pre-combine pass
>> - bit 1 selects the main combine pass
>> - bit 2 selects the new post-combine pass
So bit 0 is pass_combine2_before, bit 1 is pass_combine and bit 2 is
pass_combine2_after. But the passes are identical apart from the choice
of bit they test.
>> + /* Describes one attempt to combine instructions. */
>> + struct combination_attempt_rec
>> + {
>> + /* The instruction that we're currently trying to optimize.
>> + If the combination succeeds, we'll use this insn_info_rec
>> + to describe the new instruction. */
>> + insn_info_rec *new_home;
>> +
>> + /* The instructions we're combining, in program order. */
>> + insn_info_rec *sequence[MAX_COMBINE_INSNS];
> Can't we can this a vec in order to grow to lengths and just loop through
> merging on instructions in the vec as required?
Yeah, extending this to combining more than 2 instructions would be
future work. When that happens, this would likely end up becoming an
auto_vec<insn_info_rec *, MAX_COMBINE_INSNS>. I imagine there would
still be a fairly low compile-time limit on the number of combinations
though. E.g. current combine has a limit of 4, with even 4 being
restricted to certain high-value cases. I don't think I've ever
seen a case where 5 or more would help.
>> +/* Return true if we know that USER is the last user of RANGE. */
>> +
>> +bool
>> +combine2::known_last_use_p (live_range_rec *range, insn_info_rec *user)
>> +{
>> + if (range->last_extra_use <= user->point)
>> + return false;
>> +
>> + for (unsigned int i = 0; i < NUM_RANGE_USERS && range->users[i]; ++i)
>> + if (range->users[i] == user)
>> + return i == NUM_RANGE_USERS - 1 || !range->users[i + 1];
> Small nit and I could be wrong but do:
>
> return !range->users[i + 1] || i == NUM_RANGE_USERS - 1;
>
> Based on your code it seems that the getting to NUM_RANGE_USERS is far
> less likely.
The problem is that we'll then be accessing outside the users[] array
when i == NUM_RANGE_USERS - 1, so we have to check the limit first.
Thanks,
Richard
