This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] reload: Try alternative with swapped operands before going to the next
- From: Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>
- To: Maxim Kuvyrkov <maxim at codesourcery dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Richard Kenner <kenner at vlsi1 dot ultra dot nyu dot edu>
- Date: Wed, 16 Nov 2011 10:58:44 +0100
- Subject: Re: [PATCH] reload: Try alternative with swapped operands before going to the next
- References: <20111114172108.GA8784@bart> <03A69EF4-9A25-4BE4-B3AD-690236C7EEB2@codesourcery.com>
On 11/15/2011 11:31 PM, Maxim Kuvyrkov wrote:
> 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.
Thanks!
>> ! /* 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 ...
We are on the current alternative but the constraints array already points to the next and
has to be rewinded. But I agree that the comment is confusing. However, I'll remove this
anyway since as you said old_constraints is probably not needed at all.
>> + /* 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?
Right. I'll remove this.
> Future generations would really appreciate comments about what is going on ...
Ok. I'll add a comment saying that this just (un)swaps fields in different operand related
arrays.
Bye,
-Andreas-