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

[Luis Machado <luis.machado@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 4894 bytes --]


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.

Thanks,
Luis

[-- Attachment #2: sbfiz.diff --]
[-- Type: text/x-patch, Size: 2017 bytes --]

2018-05-10  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.
---
 gcc/config/aarch64/aarch64.md                    | 13 +++++++++++++
 gcc/testsuite/gcc.target/aarch64/lsl_asr_sbfiz.c | 24 ++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/lsl_asr_sbfiz.c

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 32a0e1f..1f943e6 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4851,6 +4851,19 @@
   [(set_attr "type" "bfx")]
 )
 
+;; Match sbfiz pattern in a shift left + shift right operation.
+
+(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")
+				      (const_int 0))
+		     (match_operand 3 "aarch64_simd_shift_imm_<mode>" "n")))]
+  ""
+  "sbfiz\\t%<w>0, %<w>1, %3, %2"
+  [(set_attr "type" "bfx")]
+)
+
 ;; When the bit position and width of the equivalent extraction add up to 32
 ;; we can use a W-reg LSL instruction taking advantage of the implicit
 ;; zero-extension of the X-reg.
diff --git a/gcc/testsuite/gcc.target/aarch64/lsl_asr_sbfiz.c b/gcc/testsuite/gcc.target/aarch64/lsl_asr_sbfiz.c
new file mode 100644
index 0000000..106433d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/lsl_asr_sbfiz.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+/* Check that a LSL followed by an ASR can be combined into a single SBFIZ
+   instruction.  */
+
+/* Using W-reg */
+
+int
+sbfiz32 (int x)
+{
+  return x << 29 >> 10;
+}
+
+/* Using X-reg */
+
+long long
+sbfiz64 (long long x)
+{
+  return x << 58 >> 20;
+}
+
+/* { dg-final { scan-assembler "sbfiz\tw" } } */
+/* { dg-final { scan-assembler "sbfiz\tx" } } */
-- 
2.7.4