Patchwork [0/8] Add optabs alternatives for insv, extv and extzv

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 27, 2012, 8:22 p.m.
Message ID <878v9mdcdl.fsf@talisman.default>
Download mbox | patch
Permalink /patch/202298/
State New
Headers show

Comments

Richard Sandiford - Nov. 27, 2012, 8:22 p.m.
Ramana Radhakrishnan <ramrad01@arm.com> writes:
>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>> Also tested by making sure that there were no changes in assembly
>> output for a set of gcc .ii files.  On the other hand, the -march=octeon
>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>> kicking in as hoped.
>
> This sequence of patches caused a regression in 
> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch 
> stack in great detail yet, but it looks like this sequence of patches 
> doesn't take the ARM volatile bitfields into account. (193600 is fine, 
> 193606 is not).

Looks like I was wrong to drop the conditions from patch 5.
Please could you try the attached?

Richard


gcc/
	* expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
	bitfields.
Ramana Radhakrishnan - Nov. 27, 2012, 10:45 p.m.
On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>> Also tested by making sure that there were no changes in assembly
>>> output for a set of gcc .ii files.  On the other hand, the -march=octeon
>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>> kicking in as hoped.
>>
>> This sequence of patches caused a regression in
>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>> stack in great detail yet, but it looks like this sequence of patches
>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>> 193606 is not).
>
> Looks like I was wrong to drop the conditions from patch 5.
> Please could you try the attached?

Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .

Ramana

>
> Richard
>
>
> gcc/
>         * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
>         bitfields.
>
> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c        2012-11-27 19:09:18.000000000 +0000
> +++ gcc/expmed.c        2012-11-27 19:09:33.314634741 +0000
> @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra
>                                 bitregion_end, MEM_ALIGN (op0),
>                                 MEM_VOLATILE_P (op0));
>    enum machine_mode best_mode;
> +  if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
> +    {
> +      while (iter.next_mode (&best_mode))
> +       if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
> +         return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
> +                                      new_bitnum);
> +      return NULL_RTX;
> +    }
> +
>    if (iter.next_mode (&best_mode))
>      {
>        /* We can use a memory in BEST_MODE.  See whether this is true for
Richard Guenther - Nov. 28, 2012, 10:25 a.m.
On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>> Also tested by making sure that there were no changes in assembly
>>>> output for a set of gcc .ii files.  On the other hand, the -march=octeon
>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>> kicking in as hoped.
>>>
>>> This sequence of patches caused a regression in
>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>> stack in great detail yet, but it looks like this sequence of patches
>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>> 193606 is not).
>>
>> Looks like I was wrong to drop the conditions from patch 5.
>> Please could you try the attached?
>
> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .

Can arm folks please re-implement strict volatile bitfields in terms of
DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
Thus, adjust stor-layout.c to compute a proper representative honoring
strict volatile bitfield semantics?

Thanks,
Richard.

> Ramana
>
>>
>> Richard
>>
>>
>> gcc/
>>         * expmed.c (adjust_bit_field_mem_for_reg): Add handling of volatile
>>         bitfields.
>>
>> Index: gcc/expmed.c
>> ===================================================================
>> --- gcc/expmed.c        2012-11-27 19:09:18.000000000 +0000
>> +++ gcc/expmed.c        2012-11-27 19:09:33.314634741 +0000
>> @@ -372,6 +372,15 @@ adjust_bit_field_mem_for_reg (enum extra
>>                                 bitregion_end, MEM_ALIGN (op0),
>>                                 MEM_VOLATILE_P (op0));
>>    enum machine_mode best_mode;
>> +  if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
>> +    {
>> +      while (iter.next_mode (&best_mode))
>> +       if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
>> +         return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
>> +                                      new_bitnum);
>> +      return NULL_RTX;
>> +    }
>> +
>>    if (iter.next_mode (&best_mode))
>>      {
>>        /* We can use a memory in BEST_MODE.  See whether this is true for
Ramana Radhakrishnan - Nov. 28, 2012, 12:06 p.m.
On 11/28/12 10:25, Richard Biener wrote:
> On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
> <ramana.gcc@googlemail.com> wrote:
>> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>>> Also tested by making sure that there were no changes in assembly
>>>>> output for a set of gcc .ii files.  On the other hand, the -march=octeon
>>>>> output for a set of mips64-linux-gnu gcc .ii files showed the optimisation
>>>>> kicking in as hoped.
>>>>
>>>> This sequence of patches caused a regression in
>>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>>> stack in great detail yet, but it looks like this sequence of patches
>>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>>> 193606 is not).
>>>
>>> Looks like I was wrong to drop the conditions from patch 5.
>>> Please could you try the attached?
>>
>> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c :( .
>
> Can arm folks please re-implement strict volatile bitfields in terms of
> DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
> Thus, adjust stor-layout.c to compute a proper representative honoring
> strict volatile bitfield semantics?

This is a regression in a feature that used to work before this patch 
went in.

I've now opened PR55516 to track this and in my book this is a P1 
regression in a primary port and will break the USB stack in the Linux 
kernel from previous experience (PR51442)

If this has to be rewritten, are you suggesting that this be done in 
time for 4.8 ? I've found the reference to your previous posts on the 
subject but on a quick perusal I don't see it as an easy fix. None of us 
understand the code as well as you do :)

OTOH I would have naively expected such a rewrite to be painful in 
stage3 even if we worked our way around this area.

Thanks,
Ramana
Richard Guenther - Nov. 28, 2012, 12:51 p.m.
On Wed, Nov 28, 2012 at 1:06 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
> On 11/28/12 10:25, Richard Biener wrote:
>>
>> On Tue, Nov 27, 2012 at 11:45 PM, Ramana Radhakrishnan
>> <ramana.gcc@googlemail.com> wrote:
>>>
>>> On Tue, Nov 27, 2012 at 8:22 PM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>>
>>>> Ramana Radhakrishnan <ramrad01@arm.com> writes:
>>>>>>
>>>>>> Tested on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.
>>>>>> Also tested by making sure that there were no changes in assembly
>>>>>> output for a set of gcc .ii files.  On the other hand, the
>>>>>> -march=octeon
>>>>>> output for a set of mips64-linux-gnu gcc .ii files showed the
>>>>>> optimisation
>>>>>> kicking in as hoped.
>>>>>
>>>>>
>>>>> This sequence of patches caused a regression in
>>>>> gcc.target/arm/volatile-bitfields-1.c . I haven't reviewed the patch
>>>>> stack in great detail yet, but it looks like this sequence of patches
>>>>> doesn't take the ARM volatile bitfields into account. (193600 is fine,
>>>>> 193606 is not).
>>>>
>>>>
>>>> Looks like I was wrong to drop the conditions from patch 5.
>>>> Please could you try the attached?
>>>
>>>
>>> Fixes volatile-bitfields-1.c but appears to break volatile-bitfields-4.c
>>> :( .
>>
>>
>> Can arm folks please re-implement strict volatile bitfields in terms of
>> DECL_BIT_FIELD_REPRESENTATIVE as I suggested elsewhere?
>> Thus, adjust stor-layout.c to compute a proper representative honoring
>> strict volatile bitfield semantics?
>
>
> This is a regression in a feature that used to work before this patch went
> in.
>
> I've now opened PR55516 to track this and in my book this is a P1 regression
> in a primary port and will break the USB stack in the Linux kernel from
> previous experience (PR51442)
>
> If this has to be rewritten, are you suggesting that this be done in time
> for 4.8 ? I've found the reference to your previous posts on the subject but
> on a quick perusal I don't see it as an easy fix. None of us understand the
> code as well as you do :)
>
> OTOH I would have naively expected such a rewrite to be painful in stage3
> even if we worked our way around this area.

It should correspond to implementing the semantics and warnings in one
single place - finish_bitfield_layout.  All the RTL expansion code can be
removed then.

As I am not familiar with the ARM strict volatile bitfield semantics I am
of no help here, but I promise to review the patch if you come up with it.

Thanks,
Richard.

> Thanks,
> Ramana
>
>

Patch

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	2012-11-27 19:09:18.000000000 +0000
+++ gcc/expmed.c	2012-11-27 19:09:33.314634741 +0000
@@ -372,6 +372,15 @@  adjust_bit_field_mem_for_reg (enum extra
 				bitregion_end, MEM_ALIGN (op0),
 				MEM_VOLATILE_P (op0));
   enum machine_mode best_mode;
+  if (MEM_VOLATILE_P (op0) && flag_strict_volatile_bitfields > 0)
+    {
+      while (iter.next_mode (&best_mode))
+	if (GET_MODE_SIZE (best_mode) == MEM_SIZE (op0))
+	  return narrow_bit_field_mem (op0, best_mode, bitsize, bitnum,
+				       new_bitnum);
+      return NULL_RTX;
+    }
+
   if (iter.next_mode (&best_mode))
     {
       /* We can use a memory in BEST_MODE.  See whether this is true for