Re: [PATCH, v2] Recognize a missed usage of a sbfiz instruction

[Luis Machado <luis dot machado at linaro dot org>]

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.