On Sun, Oct 31, 2021 at 11:02 AM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> Very many thanks to Jakub for proof-reading my patch, catching my silly
> GNU-style
> mistakes and making excellent suggestions. This revised patch incorporates
> all of
> his feedback, and has been tested on x86_64-pc-linux-gnu with make bootstrap
> and
> make -k check with no new failures.
>
> 2021-10-31 Roger Sayle <ro...@nextmovesoftware.com>
> Jakub Jelinek <ja...@redhat.com>
>
> gcc/ChangeLog
> PR target/102986
> * config/i386/i386-expand.c (ix86_expand_v1ti_to_ti,
> ix86_expand_ti_to_v1ti): New helper functions.
> (ix86_expand_v1ti_shift): Check if the amount operand is an
> integer constant, and expand as a TImode shift if it isn't.
> (ix86_expand_v1ti_rotate): Check if the amount operand is an
> integer constant, and expand as a TImode rotate if it isn't.
> (ix86_expand_v1ti_ashiftrt): New function to expand arithmetic
> right shifts of V1TImode quantities.
> * config/i386/i386-protos.h (ix86_expand_v1ti_ashift): Prototype.
> * config/i386/sse.md (ashlv1ti3, lshrv1ti3): Change constraints
> to QImode general_operand, and let the helper functions lower
> shifts by non-constant operands, as TImode shifts. Make
> conditional on TARGET_64BIT.
> (ashrv1ti3): New expander calling ix86_expand_v1ti_ashiftrt.
> (rotlv1ti3, rotrv1ti3): Change shift operand to QImode.
> Make conditional on TARGET_64BIT.
>
> gcc/testsuite/ChangeLog
> PR target/102986
> * gcc.target/i386/sse2-v1ti-ashiftrt-1.c: New test case.
> * gcc.target/i386/sse2-v1ti-ashiftrt-2.c: New test case.
> * gcc.target/i386/sse2-v1ti-ashiftrt-3.c: New test case.
> * gcc.target/i386/sse2-v1ti-shift-2.c: New test case.
> * gcc.target/i386/sse2-v1ti-shift-3.c: New test case.
>
> Thanks.
> Roger
> --
>
> -----Original Message-----
> From: Jakub Jelinek <ja...@redhat.com>
> Sent: 30 October 2021 11:30
> To: Roger Sayle <ro...@nextmovesoftware.com>
> Cc: 'GCC Patches' <gcc-patches@gcc.gnu.org>; 'Uros Bizjak'
> <ubiz...@gmail.com>
> Subject: Re: [PATCH] x86_64: Expand ashrv1ti (and PR target/102986)
>
> On Sat, Oct 30, 2021 at 11:16:41AM +0100, Roger Sayle wrote:
> > 2021-10-30 Roger Sayle <ro...@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> > PR target/102986
> > * config/i386/i386-expand.c (ix86_expand_v1ti_to_ti,
> > ix86_expand_ti_to_v1ti): New helper functions.
> > (ix86_expand_v1ti_shift): Check if the amount operand is an
> > integer constant, and expand as a TImode shift if it isn't.
> > (ix86_expand_v1ti_rotate): Check if the amount operand is an
> > integer constant, and expand as a TImode rotate if it isn't.
> > (ix86_expand_v1ti_ashiftrt): New function to expand arithmetic
> > right shifts of V1TImode quantities.
> > * config/i386/i386-protos.h (ix86_expand_v1ti_ashift): Prototype.
> > * config/i386/sse.md (ashlv1ti3, lshrv1ti3): Change constraints
> > to QImode general_operand, and let the helper functions lower
> > shifts by non-constant operands, as TImode shifts.
> > (ashrv1ti3): New expander calling ix86_expand_v1ti_ashiftrt.
> > (rotlv1ti3, rotrv1ti3): Change shift operand to QImode.
> >
> > gcc/testsuite/ChangeLog
> > PR target/102986
> > * gcc.target/i386/sse2-v1ti-ashiftrt-1.c: New test case.
> > * gcc.target/i386/sse2-v1ti-ashiftrt-2.c: New test case.
> > * gcc.target/i386/sse2-v1ti-ashiftrt-3.c: New test case.
> > * gcc.target/i386/sse2-v1ti-shift-2.c: New test case.
> > * gcc.target/i386/sse2-v1ti-shift-3.c: New test case.
> >
> > Sorry again for the breakage in my last patch. I wasn't testing things
> > that shouldn't have been affected/changed.
>
> Not a review, will defer that to Uros, but just nits:
>
> > +/* Expand move of V1TI mode register X to a new TI mode register. */
> > +static rtx ix86_expand_v1ti_to_ti (rtx x)
>
> ix86_expand_v1ti_to_ti should be at the start of next line, so static rtx
> ix86_expand_v1ti_to_ti (rtx x)
>
> Ditto for other functions and also in functions you've added by the previous
> patch.
> > + emit_insn (code == ASHIFT ? gen_ashlti3(tmp2, tmp1, operands[2])
> > + : gen_lshrti3(tmp2, tmp1, operands[2]));
>
> Space before ( twice.
>
> > + emit_insn (code == ROTATE ? gen_rotlti3(tmp2, tmp1, operands[2])
> > + : gen_rotrti3(tmp2, tmp1, operands[2]));
>
> Likewise.
>
> > + emit_insn (gen_ashrti3(tmp2, tmp1, operands[2]));
>
> Similarly.
>
> Also, I wonder for all these patterns (previously and now added), shouldn't
> they have && TARGET_64BIT in conditions? I mean, we don't really support
> scalar TImode for ia32, but VALID_SSE_REG_MODE includes V1TImode and while
> the constant shifts can be done, I think the variable shifts can't, there
> are no TImode shift patterns...
- (match_operand:SI 2 "const_int_operand")))]
- "TARGET_SSE2"
+ (match_operand:QI 2 "general_operand")))]
+ "TARGET_SSE2 && TARGET_64BIT"
I wonder if this change is too restrictive, as it disables V1TI shifts
by constant on 32bit targets. Perhaps we can introduce a conditional
predicate, like:
(define_predicate "shiftv1ti_input_operand"
(if_then_else (match_test "TARGET_64BIT")
(match_operand 0 "general_operand")
(match_operand 0 "const_int_operand")))
However, I'm not familiar with how the middle-end behaves with the
above approach - will it try to put the constant in a register under
some circumstances and consequently fail the expansion?
And one mandatory :) nit:
+ rtx tmp1 = ix86_expand_v1ti_to_ti (op1);
+ rtx tmp2 = gen_reg_rtx (TImode);
+ emit_insn (code == ASHIFT ? gen_ashlti3 (tmp2, tmp1, operands[2])
+ : gen_lshrti3 (tmp2, tmp1, operands[2]));
+ rtx tmp3 = ix86_expand_ti_to_v1ti (tmp2);
+ emit_move_insn (operands[0], tmp3);
+ return;
I'd write this as:
rtx tmp1 = ix86_expand_v1ti_to_ti (op1);
rtx tmp2 = gen_reg_rtx (TImode);
rtx (*shift) (rtx, rtx, rtx)
= (code == ASHIFT) ? gen_ashlti3 : gen_lshrti3;
emit_insn (shift (tmp2, tmp1, operands[2]));
rtx tmp3 = ix86_expand_ti_to_v1ti (tmp2);
emit_move_insn (operands[0], tmp3);
return;
Otherwise LGTM (and kudos for writing out all those sequences).
Thanks,
Uros.
