On 15/11/2011, at 6:21 AM, Andreas Krebbel wrote:
> Hi,
>
> find_reloads currently loops over all alternatives in an insn and
> restarts the whole process after swapping commutative operands. This
> together with the early exit for a perfectly matching alternative
> leads to an behavior which does not exactly matches the manual.
>
> From http://gcc.gnu.org/onlinedocs/gccint/Multi_002dAlternative.html
> "If two alternatives need the same amount of copying, the one that
> comes first is chosen."
>
> An earlier alternative could be turned into a perfect match by
> swapping the operands. But this will not happen if a later
> alternative already provides a perfect match without swapping the
> operands.
...
>
> The attached patch changes the loop nesting in find_reloads in order
> to try each alternative with swapped operands before picking the next.
I have eye-balled this patch for good half-an-hour and couldn't poke any holes
in it. I can't approve this patch, but below are some review comments. Mostly
these are suggested comments to make reload easier to understand for future
generations.
>
> Bootstrapped on s390x, x86_64 and PPC64. No regressions
>
> Ok for mainline?
Good portion of the code you're changing was written by Richard K. ages ago, so
CC'ing him to get his approval.
>
> Bye,
>
> -Andreas-
>
>
> 2011-11-14 Andreas Krebbel <andreas.kreb...@de.ibm.com>
>
> * reload.c (find_reloads): Change the loop nesting when trying an
> alternative with swapped operands.
>
>
> The first version of the patch is without adjusting the indention in
> order to allow for easier review.
>
> Index: gcc/reload.c
> ===================================================================
> *** gcc/reload.c.orig
> --- gcc/reload.c
> *************** find_reloads (rtx insn, int replace, int
> *** 2592,2598 ****
> char this_alternative_offmemok[MAX_RECOG_OPERANDS];
> char this_alternative_earlyclobber[MAX_RECOG_OPERANDS];
> int this_alternative_matches[MAX_RECOG_OPERANDS];
> - int swapped;
> reg_class_t goal_alternative[MAX_RECOG_OPERANDS];
> int this_alternative_number;
> int goal_alternative_number = 0;
> --- 2592,2597 ----
> *************** find_reloads (rtx insn, int replace, int
> *** 2938,2946 ****
>
> best = MAX_RECOG_OPERANDS * 2 + 600;
>
> - swapped = 0;
> goal_alternative_swapped = 0;
> - try_swapped:
>
> /* The constraints are made of several alternatives.
> Each operand's constraint looks like foo,bar,... with commas
> --- 2937,2943 ----
> *************** find_reloads (rtx insn, int replace, int
> *** 2953,2958 ****
> --- 2950,2975 ----
> this_alternative_number < n_alternatives;
> this_alternative_number++)
> {
> + int swapped;
> + const char *old_constraints[MAX_RECOG_OPERANDS];
> +
/* Skip disabled alternatives. */
> + if (!recog_data.alternative_enabled_p[this_alternative_number])
> + {
> + int i;
> +
> + for (i = 0; i < recog_data.n_operands; i++)
> + constraints[i] = skip_alternative (constraints[i]);
> +
> + continue;
> + }
> +
> + /* If insn is commutative (it's safe to exchange a certain pair
> + of operands) then we need to try each alternative twice, the
> + second time matching those two operands as if we had
> + exchanged them. To do this, really exchange them in
> + operands. */
> + for (swapped = 0; swapped < (commutative >= 0 ? 2 : 1); swapped++)
> + {
> /* Loop over operands for one constraint alternative. */
> /* LOSERS counts those that don't fit this alternative
> and would require loading. */
> *************** find_reloads (rtx insn, int replace, int
> *** 2968,2982 ****
> a bad register class to only count 1/3 as much. */
> int reject = 0;
>
> ! if (!recog_data.alternative_enabled_p[this_alternative_number])
> ! {
> ! int i;
>
> ! for (i = 0; i < recog_data.n_operands; i++)
> ! constraints[i] = skip_alternative (constraints[i]);
>
> ! continue;
> ! }
>
> this_earlyclobber = 0;
>
> --- 2985,3021 ----
> a bad register class to only count 1/3 as much. */
> int reject = 0;
>
> ! if (swapped)
/* Trying THIS_ALTERNATIVE with commutative operands swapped.
Do the swapping. */
> ! {
> ! enum reg_class tclass;
> ! int t;
>
> ! recog_data.operand[commutative] = substed_operand[commutative +
> 1];
> ! recog_data.operand[commutative + 1] =
> substed_operand[commutative];
> ! /* Swap the duplicates too. */
> ! for (i = 0; i < recog_data.n_dups; i++)
> ! if (recog_data.dup_num[i] == commutative
> ! || recog_data.dup_num[i] == commutative + 1)
> ! *recog_data.dup_loc[i]
> ! = recog_data.operand[(int) recog_data.dup_num[i]];
> !
> ! tclass = preferred_class[commutative];
> ! preferred_class[commutative] = preferred_class[commutative + 1];
> ! preferred_class[commutative + 1] = tclass;
> !
> ! t = pref_or_nothing[commutative];
> ! pref_or_nothing[commutative] = pref_or_nothing[commutative + 1];
> ! pref_or_nothing[commutative + 1] = t;
> !
> ! t = address_reloaded[commutative];
> ! address_reloaded[commutative] = address_reloaded[commutative + 1];
> ! address_reloaded[commutative + 1] = t;
>
> ! /* Restore the constraint pointers to the previous
> ! alternative. */
> ! for (i = 0; i < noperands; i++)
> ! constraints[i] = old_constraints[i];
I'm not sure the comment is precise. We are still on the current alternative,
we are just rolling back the advancement constraints[] done ...
> ! }
>
> this_earlyclobber = 0;
>
> *************** find_reloads (rtx insn, int replace, int
> *** 3475,3480 ****
> --- 3514,3523 ----
> }
> while ((p += len), c);
>
> + /* Make a backup of the old constraint pointer since we
> + will need it when retrying the alternative with
> + swapped operands. */
> + old_constraints[i] = constraints[i];
> constraints[i] = p;
... here.
Furthermore, as constraints[i] seem not be used down the function, can't we
just condition constraints[i] update on "if (swapped == (commutative >= 0 ? 1 :
0))" and avoid old_constraints altogether?
If we do need to stick with old_constraints, then I suggest adding "if
(!swapped)" before it just to make it explicit that those values are used only
by "if (swapped)" clauses above.
>
> /* If this operand could be handled with a reg,
> *************** find_reloads (rtx insn, int replace, int
> *** 3689,3695 ****
> if (losers == 0)
> {
> /* Unswap these so that they are never swapped at `finish'. */
> ! if (commutative >= 0)
> {
> recog_data.operand[commutative] = substed_operand[commutative];
> recog_data.operand[commutative + 1]
> --- 3732,3738 ----
> if (losers == 0)
> {
> /* Unswap these so that they are never swapped at `finish'. */
> ! if (swapped)
> {
> recog_data.operand[commutative] = substed_operand[commutative];
> recog_data.operand[commutative + 1]
> *************** find_reloads (rtx insn, int replace, int
> *** 3742,3803 ****
> goal_earlyclobber = this_earlyclobber;
> }
> }
> ! }
>
> ! /* If insn is commutative (it's safe to exchange a certain pair of
> operands)
> ! then we need to try each alternative twice,
> ! the second time matching those two operands
> ! as if we had exchanged them.
> ! To do this, really exchange them in operands.
> !
> ! If we have just tried the alternatives the second time,
> ! return operands to normal and drop through. */
> !
> ! if (commutative >= 0)
> ! {
> ! swapped = !swapped;
> ! if (swapped)
> ! {
> ! enum reg_class tclass;
> ! int t;
> !
> ! recog_data.operand[commutative] = substed_operand[commutative + 1];
> ! recog_data.operand[commutative + 1] = substed_operand[commutative];
> ! /* Swap the duplicates too. */
> ! for (i = 0; i < recog_data.n_dups; i++)
> ! if (recog_data.dup_num[i] == commutative
> ! || recog_data.dup_num[i] == commutative + 1)
> ! *recog_data.dup_loc[i]
> ! = recog_data.operand[(int) recog_data.dup_num[i]];
> !
> ! tclass = preferred_class[commutative];
> ! preferred_class[commutative] = preferred_class[commutative + 1];
> ! preferred_class[commutative + 1] = tclass;
> !
> ! t = pref_or_nothing[commutative];
> ! pref_or_nothing[commutative] = pref_or_nothing[commutative + 1];
> ! pref_or_nothing[commutative + 1] = t;
> !
> ! t = address_reloaded[commutative];
> ! address_reloaded[commutative] = address_reloaded[commutative + 1];
> ! address_reloaded[commutative + 1] = t;
> !
> ! memcpy (constraints, recog_data.constraints,
> ! noperands * sizeof (const char *));
> ! goto try_swapped;
> ! }
> ! else
> ! {
> ! recog_data.operand[commutative] = substed_operand[commutative];
> ! recog_data.operand[commutative + 1]
> ! = substed_operand[commutative + 1];
> ! /* Unswap the duplicates too. */
> ! for (i = 0; i < recog_data.n_dups; i++)
> ! if (recog_data.dup_num[i] == commutative
> ! || recog_data.dup_num[i] == commutative + 1)
> ! *recog_data.dup_loc[i]
> ! = recog_data.operand[(int) recog_data.dup_num[i]];
> ! }
> }
>
> /* The operands don't meet the constraints.
> --- 3785,3817 ----
> goal_earlyclobber = this_earlyclobber;
> }
> }
> ! if (swapped)
> ! {
> ! enum reg_class tclass;
> ! int t;
>
>
Future generations would really appreciate comments about what is going on ...
... here, ...
> ! recog_data.operand[commutative] = substed_operand[commutative];
> ! recog_data.operand[commutative + 1] = substed_operand[commutative
> + 1];
> ! /* Swap the duplicates too. */
> ! for (i = 0; i < recog_data.n_dups; i++)
> ! if (recog_data.dup_num[i] == commutative
> ! || recog_data.dup_num[i] == commutative + 1)
> ! *recog_data.dup_loc[i]
> ! = recog_data.operand[(int) recog_data.dup_num[i]];
> !
... here, ...
>
> ! tclass = preferred_class[commutative];
> ! preferred_class[commutative] = preferred_class[commutative + 1];
> ! preferred_class[commutative + 1] = tclass;
> !
... here, ...
>
> ! t = pref_or_nothing[commutative];
> ! pref_or_nothing[commutative] = pref_or_nothing[commutative + 1];
> ! pref_or_nothing[commutative + 1] = t;
> !
... and here.
>
> ! t = address_reloaded[commutative];
> ! address_reloaded[commutative] = address_reloaded[commutative + 1];
> ! address_reloaded[commutative + 1] = t;
> ! }
> ! }
> }
>
> /* The operands don't meet the constraints.
>
>
Thank you,
--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
