This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, v2] Recognize a missed usage of a sbfiz instruction
On Mon, May 14, 2018 at 03:41:34PM -0500, Luis Machado wrote:
> On 05/11/2018 06:46 AM, Kyrill Tkachov wrote:
> > Hi Luis,
> >
> > On 10/05/18 11:31, Luis Machado wrote:
> >>
> >> On 05/09/2018 10:44 AM, Kyrill Tkachov wrote:
> >>>
> >>> On 09/05/18 13:30, Luis Machado wrote:
> >>>> Hi Kyrill,
> >>>>
> >>>> On 05/08/2018 11:09 AM, Kyrill Tkachov wrote:
> >>>>> Hi Luis,
> >>>>>
> >>>>> On 07/05/18 15:28, Luis Machado wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 02/08/2018 10:45 AM, Luis Machado wrote:
> >>>>>>> Hi Kyrill,
> >>>>>>>
> >>>>>>> On 02/08/2018 09:48 AM, Kyrill Tkachov wrote:
> >>>>>>>> Hi Luis,
> >>>>>>>>
> >>>>>>>> On 06/02/18 15:04, Luis Machado wrote:
> >>>>>>>>> Thanks for the feedback Kyrill. I've adjusted the v2 patch
> >>>>>>>>> based on your
> >>>>>>>>> suggestions and re-tested the changes. Everything is still sane.
> >>>>>>>>
> >>>>>>>> Thanks! This looks pretty good to me.
> >>>>>>>>
> >>>>>>>>> Since this is ARM-specific and fairly specific, i wonder if it
> >>>>>>>>> would be
> >>>>>>>>> reasonable to consider it for inclusion at the current stage.
> >>>>>>>>
> >>>>>>>> It is true that the target maintainers can choose to take
> >>>>>>>> such patches at any stage. However, any patch at this stage
> >>>>>>>> increases
> >>>>>>>> the risk of regressions being introduced and these regressions
> >>>>>>>> can come bite us in ways that are very hard to anticipate.
> >>>>>>>>
> >>>>>>>> Have a look at some of the bugs in bugzilla (or a quick scan of
> >>>>>>>> the gcc-bugs list)
> >>>>>>>> for examples of the ways that things can go wrong with any of
> >>>>>>>> the myriad of GCC components
> >>>>>>>> and the unexpected ways in which they can interact.
> >>>>>>>>
> >>>>>>>> For example, I am now working on what I initially thought was a
> >>>>>>>> one-liner fix for
> >>>>>>>> PR 84164 but it has expanded into a 3-patch series with a midend
> >>>>>>>> component and
> >>>>>>>> target-specific changes for 2 ports.
> >>>>>>>>
> >>>>>>>> These issues are very hard to catch during review and normal
> >>>>>>>> testing, and can sometimes take months of deep testing by
> >>>>>>>> fuzzing and massive codebase rebuilds to expose, so the closer
> >>>>>>>> the commit is to a release
> >>>>>>>> the higher the risk is that an obscure edge case will be
> >>>>>>>> unnoticed and unfixed in the release.
> >>>>>>>>
> >>>>>>>> So the priority at this stage is to minimise the risk of
> >>>>>>>> destabilising the codebase,
> >>>>>>>> as opposed to taking in new features and desirable performance
> >>>>>>>> improvements (like your patch!)
> >>>>>>>>
> >>>>>>>> That is the rationale for delaying committing such changes until
> >>>>>>>> the start
> >>>>>>>> of GCC 9 development. But again, this is up to the aarch64
> >>>>>>>> maintainers.
> >>>>>>>> I'm sure the patch will be a perfectly fine and desirable commit
> >>>>>>>> for GCC 9.
> >>>>>>>> This is just my perspective as maintainer of the arm port.
> >>>>>>>
> >>>>>>> Thanks. Your explanation makes the situation pretty clear and it
> >>>>>>> sounds very reasonable. I'll put the patch on hold until
> >>>>>>> development is open again.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Luis
> >>>>>>
> >>>>>> With GCC 9 development open, i take it this patch is worth
> >>>>>> considering again?
> >>>>>>
> >>>>>
> >>>>> Yes, I believe the latest version is at:
> >>>>> https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00239.html ?
> >>>>>
> >>>>> +(define_insn "*ashift<mode>_extv_bfiz"
> >>>>> + [(set (match_operand:GPI 0 "register_operand" "=r")
> >>>>> + (ashift:GPI (sign_extract:GPI (match_operand:GPI 1
> >>>>> "register_operand" "r")
> >>>>> + (match_operand 2
> >>>>> "aarch64_simd_shift_imm_offset_<mode>" "n")
> >>>>> + (match_operand 3
> >>>>> "aarch64_simd_shift_imm_<mode>" "n"))
> >>>>> + (match_operand 4 "aarch64_simd_shift_imm_<mode>" "n")))]
> >>>>> + ""
> >>>>> + "sbfiz\\t%<w>0, %<w>1, %4, %2"
> >>>>> + [(set_attr "type" "bfx")]
> >>>>> +)
> >>>>> +
> >>>>
> >>>> Indeed.
> >>>>
> >>>>>
> >>>>> Can you give a bit more information about what are the values for
> >>>>> operands 2,3 and 4 in your example testcases?
> >>>>
> >>>> For sbfiz32 we have 3, 0 and 19 respectively. For sbfiz64 we have 6,
> >>>> 0 and 38.
> >>>>
> >>>>> I'm trying to understand why the value of operand 3 (the bit
> >>>>> position the sign-extract starts from) doesn't get validated
> >>>>> in any way and doesn't play any role in the output...
> >>>>
> >>>> This may be an oversight. It seems operand 3 will always be 0 in
> >>>> this particular case i'm covering. It starts from 0, gets shifted x
> >>>> bits to the left and then y < x bits to the right). The operation is
> >>>> essentially an ashift of the bitfield followed by a sign-extension
> >>>> of the msb of the bitfield being extracted.
> >>>>
> >>>> Having a non-zero operand 3 from RTL means the shift amount won't
> >>>> translate directly to operand 3 of sbfiz (the position). Then we'd
> >>>> need to do a calculation where we take into account operand 3 from RTL.
> >>>>
> >>>> I'm wondering when such a RTL pattern, with a non-zero operand 3,
> >>>> would be generated though.
> >>>
> >>> I think it's best to enforce that operand 3 is a zero. Maybe just
> >>> match const_int 0 here directly.
> >>> Better safe than sorry with these things.
> >>
> >> Indeed. I've updated the original patch with that change now.
> >>
> >> Bootstrapped and regtested on aarch64-linux.
> >>
> >
> > I think you might also want to enforce that the sum of operands 2 and 3
> > doesn't exceed the width of the mode
> > (see other patterns in aarch64.md that generate bfx-style instructions
> > for similar restrictions).
>
> Updated now in the attached patch. Everything still sane with bootstrap
> and testsuite on aarch64-linux.
>
> >
> > Otherwise the patch looks good to me (it will still need approval from a
> > maintainer).
> >
> > For the future I think there's also an opportunity to match the SImode
> > version of this pattern that zero-extends to DImode,
> > making use of the implicit zero-extension that writing to a W-dreg
> > provides. But that would be a separate patch.
>
> Indeed. I'll have a TODO for this.
>
> >
> > Thanks, and sorry for the respins,
>
> Thanks for reviewing it!
OK.
Thanks,
James
> 2018-05-14 Luis Machado <luis.machado@linaro.org>
>
> gcc/
> * config/aarch64/aarch64.md (*ashift<mode>_extv_bfiz): New pattern.
>
> gcc/testsuite/
> * gcc.target/aarch64/lsl_asr_sbfiz.c: New test.