This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] - fix optimizations on vector ROTATERT in combine.c
- From: Aldy Hernandez <aldyh at redhat dot com>
- To: "Igor Shevlyakov" <igor at microunity dot com>
- Cc: <gcc-patches at gcc dot gnu dot org>
- Date: 28 Aug 2002 10:11:46 -0400
- Subject: Re: [PATCH] - fix optimizations on vector ROTATERT in combine.c
- References: <001401c24d4f$fb9ed200$1e0756c0@medusa>
>>>>> "Igor" == Igor Shevlyakov <igor@microunity.com> writes:
> When combine.c (simplify_shift_const) converting ROTATERT to ROTATE new
> shift count is calculated incorrectly for vector operands since each
> element is rotated by count not whole vector as bit-stream. This patch
> fixes it
Hmmm, I didn't know we had any ports that used rotate* on SIMD
operands. And I don't think we've even defined what a rotate on SIMD
operands is supposed to do.
In any case, here are a few general comments related to style.
> 2002-08-26 Igor Shevlyakov <igor@microunity.com>
> * combine.c (simplify_shift_const): Converting from ROTATERT to
> ROTATE for
> vector operands calculated shift count incorrectly.
ChangeLog entries should specify what the new behavior is, not what
was broken. So something like this would be better:
Calculate rotate count correctly for vector operands.
Please read the GNU Coding Standards at:
http://www.gnu.org/prep/standards_toc.html
> /* Convert ROTATERT to ROTATE. */
> ! if (code == ROTATERT)
> ! code = ROTATE, count = GET_MODE_BITSIZE (result_mode) - count;
> /* We need to determine what mode we will do the shift in. If
> the
> shift is a right shift or a ROTATE, we must always do it in the
> mode
> --- 9041,9053 ----
> break;
> /* Convert ROTATERT to ROTATE. */
> ! if (code == ROTATERT) {
Curlies go on a separate line.
> ! code = ROTATE;
> ! if (VECTOR_MODE_P(result_mode))
Space after VECTOR_MODE_P.
> ! count = GET_MODE_BITSIZE (result_mode) /
> GET_MODE_NUNITS(result_mode) - count;
> ! else
> ! count = GET_MODE_BITSIZE (result_mode) - count;
> ! }
You need a testcase. You also need to specify how you tested this
(bootstrapped and regression tested on which platforms).
Otherwise, this looks reasonable, but a global maintainer will have to
approve this.
Cheers.
Aldy