diff mbox series

[i386] : Fix PR82725, ICE in change_address_1, at emit-rtl.c

Message ID CAFULd4YM54ZQ=Vxbwq8wZQDhfZAiFg+8WhO-Gb8A1svEBRm7AQ@mail.gmail.com
State New
Headers show
Series [i386] : Fix PR82725, ICE in change_address_1, at emit-rtl.c | expand

Commit Message

Uros Bizjak Oct. 29, 2017, 7:38 a.m. UTC 
Hello!

split_double_mode tries to split TImode address:

(insn 10 2 11 2 (set (reg/i:TI 0 ax)
        (mem/u/c:TI (const:DI (unspec:DI [
                        (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
                    ] UNSPEC_NTPOFF)) [1 cache+0 S16 A64 AS1]))
to:

(const:DI (unspec:DI [
                (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
            ] UNSPEC_NTPOFF))

and

(const:DI (plus:DI (unspec:DI [
                (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
            ] UNSPEC_NTPOFF)
        (const_int 8 [0x8])))

but the later RTX is not recognized as a valid address.

Attached patch allows offsetted UNSPEC_DTPOFF/UNSPEC_NTPOFF
relocations in legitimate_pic_address_disp_p. In fact, these are
already allowed in x86_64_immediate_operand, with the immediate
limited to SImode values.

2017-10-29  Uros Bizjak  <ubizjak@gmail.com>

    PR target/82725
    * config/i386/i386.c (legitimate_pic_address_disp_p): Allow
    UNSPEC_DTPOFF and UNSPEC_NTPOFF with SImode immediate offset.

testsuite/ChangeLog:

2017-10-29  Uros Bizjak  <ubizjak@gmail.com>

    PR target/82725
    * g++.dg/pr82725.C: New test.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Jakub, HJ, do you have any comments on the patch?

Uros.

Comments

H.J. Lu Oct. 29, 2017, 4:55 p.m. UTC | #1 
On Sun, Oct 29, 2017 at 12:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
> split_double_mode tries to split TImode address:
>
> (insn 10 2 11 2 (set (reg/i:TI 0 ax)
>         (mem/u/c:TI (const:DI (unspec:DI [
>                         (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>                     ] UNSPEC_NTPOFF)) [1 cache+0 S16 A64 AS1]))
> to:
>
> (const:DI (unspec:DI [
>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>             ] UNSPEC_NTPOFF))
>
> and
>
> (const:DI (plus:DI (unspec:DI [
>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>             ] UNSPEC_NTPOFF)
>         (const_int 8 [0x8])))
>
> but the later RTX is not recognized as a valid address.
>
> Attached patch allows offsetted UNSPEC_DTPOFF/UNSPEC_NTPOFF
> relocations in legitimate_pic_address_disp_p. In fact, these are
> already allowed in x86_64_immediate_operand, with the immediate
> limited to SImode values.
>
> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>
>     PR target/82725
>     * config/i386/i386.c (legitimate_pic_address_disp_p): Allow
>     UNSPEC_DTPOFF and UNSPEC_NTPOFF with SImode immediate offset.
>
> testsuite/ChangeLog:
>
> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>
>     PR target/82725
>     * g++.dg/pr82725.C: New test.
>
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> Jakub, HJ, do you have any comments on the patch?
>
> Uros.

2 questions:

1.  Why isn't legitimate_pic_address_disp_p a static function?
2.  Should we add a legitimate_address_disp_p function to
handle both PIC and non-PIC displacement which is also
used by x86_64_immediate_operand?
Uros Bizjak Oct. 29, 2017, 5:25 p.m. UTC | #2 
On Sun, Oct 29, 2017 at 5:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Oct 29, 2017 at 12:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Hello!
>>
>> split_double_mode tries to split TImode address:
>>
>> (insn 10 2 11 2 (set (reg/i:TI 0 ax)
>>         (mem/u/c:TI (const:DI (unspec:DI [
>>                         (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>                     ] UNSPEC_NTPOFF)) [1 cache+0 S16 A64 AS1]))
>> to:
>>
>> (const:DI (unspec:DI [
>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>             ] UNSPEC_NTPOFF))
>>
>> and
>>
>> (const:DI (plus:DI (unspec:DI [
>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>             ] UNSPEC_NTPOFF)
>>         (const_int 8 [0x8])))
>>
>> but the later RTX is not recognized as a valid address.
>>
>> Attached patch allows offsetted UNSPEC_DTPOFF/UNSPEC_NTPOFF
>> relocations in legitimate_pic_address_disp_p. In fact, these are
>> already allowed in x86_64_immediate_operand, with the immediate
>> limited to SImode values.
>>
>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     PR target/82725
>>     * config/i386/i386.c (legitimate_pic_address_disp_p): Allow
>>     UNSPEC_DTPOFF and UNSPEC_NTPOFF with SImode immediate offset.
>>
>> testsuite/ChangeLog:
>>
>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     PR target/82725
>>     * g++.dg/pr82725.C: New test.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>
>> Jakub, HJ, do you have any comments on the patch?
>>
>> Uros.
>
> 2 questions:
>
> 1.  Why isn't legitimate_pic_address_disp_p a static function?

Looks like it was used from .md files in the past (it still has a
prototype in i386-protos.h). It can be made static, but I don't want
to mix cleanups and bugfixes in the same patch.

> 2.  Should we add a legitimate_address_disp_p function to
> handle both PIC and non-PIC displacement which is also
> used by x86_64_immediate_operand?

There are opportunities in x86 PIC code for a considerable cleanup.
The code looks like x86_64 part was bolted on the i386 code, with some
parts of TARGET_64BIT code duplicating generic x86 code.

Uros.
Uros Bizjak Oct. 29, 2017, 5:28 p.m. UTC | #3 
On Sun, Oct 29, 2017 at 6:25 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sun, Oct 29, 2017 at 5:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Oct 29, 2017 at 12:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> Hello!
>>>
>>> split_double_mode tries to split TImode address:
>>>
>>> (insn 10 2 11 2 (set (reg/i:TI 0 ax)
>>>         (mem/u/c:TI (const:DI (unspec:DI [
>>>                         (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>                     ] UNSPEC_NTPOFF)) [1 cache+0 S16 A64 AS1]))
>>> to:
>>>
>>> (const:DI (unspec:DI [
>>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>             ] UNSPEC_NTPOFF))
>>>
>>> and
>>>
>>> (const:DI (plus:DI (unspec:DI [
>>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>             ] UNSPEC_NTPOFF)
>>>         (const_int 8 [0x8])))
>>>
>>> but the later RTX is not recognized as a valid address.
>>>
>>> Attached patch allows offsetted UNSPEC_DTPOFF/UNSPEC_NTPOFF
>>> relocations in legitimate_pic_address_disp_p. In fact, these are
>>> already allowed in x86_64_immediate_operand, with the immediate
>>> limited to SImode values.
>>>
>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     PR target/82725
>>>     * config/i386/i386.c (legitimate_pic_address_disp_p): Allow
>>>     UNSPEC_DTPOFF and UNSPEC_NTPOFF with SImode immediate offset.
>>>
>>> testsuite/ChangeLog:
>>>
>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     PR target/82725
>>>     * g++.dg/pr82725.C: New test.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>
>>> Jakub, HJ, do you have any comments on the patch?
>>>
>>> Uros.
>>
>> 2 questions:
>>
>> 1.  Why isn't legitimate_pic_address_disp_p a static function?
>
> Looks like it was used from .md files in the past (it still has a
> prototype in i386-protos.h). It can be made static, but I don't want
> to mix cleanups and bugfixes in the same patch.
>
>> 2.  Should we add a legitimate_address_disp_p function to
>> handle both PIC and non-PIC displacement which is also
>> used by x86_64_immediate_operand?
>
> There are opportunities in x86 PIC code for a considerable cleanup.
> The code looks like x86_64 part was bolted on the i386 code, with some
> parts of TARGET_64BIT code duplicating generic x86 code.

That is, "... opportunities in x86 relocation handling code ...".

Uros.
H.J. Lu Oct. 29, 2017, 5:59 p.m. UTC | #4 
On Sun, Oct 29, 2017 at 10:28 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sun, Oct 29, 2017 at 6:25 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Sun, Oct 29, 2017 at 5:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Sun, Oct 29, 2017 at 12:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>> Hello!
>>>>
>>>> split_double_mode tries to split TImode address:
>>>>
>>>> (insn 10 2 11 2 (set (reg/i:TI 0 ax)
>>>>         (mem/u/c:TI (const:DI (unspec:DI [
>>>>                         (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>>                     ] UNSPEC_NTPOFF)) [1 cache+0 S16 A64 AS1]))
>>>> to:
>>>>
>>>> (const:DI (unspec:DI [
>>>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>>             ] UNSPEC_NTPOFF))
>>>>
>>>> and
>>>>
>>>> (const:DI (plus:DI (unspec:DI [
>>>>                 (symbol_ref:DI ("_ZZ7tempDirvE5cache") [flags 0x2a])
>>>>             ] UNSPEC_NTPOFF)
>>>>         (const_int 8 [0x8])))
>>>>
>>>> but the later RTX is not recognized as a valid address.
>>>>
>>>> Attached patch allows offsetted UNSPEC_DTPOFF/UNSPEC_NTPOFF
>>>> relocations in legitimate_pic_address_disp_p. In fact, these are
>>>> already allowed in x86_64_immediate_operand, with the immediate
>>>> limited to SImode values.
>>>>
>>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>>
>>>>     PR target/82725
>>>>     * config/i386/i386.c (legitimate_pic_address_disp_p): Allow
>>>>     UNSPEC_DTPOFF and UNSPEC_NTPOFF with SImode immediate offset.
>>>>
>>>> testsuite/ChangeLog:
>>>>
>>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>>
>>>>     PR target/82725
>>>>     * g++.dg/pr82725.C: New test.
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>>
>>>> Jakub, HJ, do you have any comments on the patch?
>>>>
>>>> Uros.
>>>
>>> 2 questions:
>>>
>>> 1.  Why isn't legitimate_pic_address_disp_p a static function?
>>
>> Looks like it was used from .md files in the past (it still has a
>> prototype in i386-protos.h). It can be made static, but I don't want
>> to mix cleanups and bugfixes in the same patch.

Make sense.

>>> 2.  Should we add a legitimate_address_disp_p function to
>>> handle both PIC and non-PIC displacement which is also
>>> used by x86_64_immediate_operand?
>>
>> There are opportunities in x86 PIC code for a considerable cleanup.
>> The code looks like x86_64 part was bolted on the i386 code, with some
>> parts of TARGET_64BIT code duplicating generic x86 code.
>
> That is, "... opportunities in x86 relocation handling code ...".
>

I think x86 displacement check should be put in a function and used by
both x86_64_immediate_operand and legitimate_pic_address_disp_p.
Paolo Carlini Oct. 30, 2017, 6:56 p.m. UTC | #5 
Hi,

On 29/10/2017 18:59, H.J. Lu wrote:
>
>>>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>>>
>>>>>      PR target/82725
>>>>>      * g++.dg/pr82725.C: New test.
The new testcase is failing for everybody in gnu++98 mode. I think you 
want to move it to the cpp0x directory and use the appropriate DejaGnu 
incantations.

Paolo.
Uros Bizjak Oct. 30, 2017, 8:06 p.m. UTC | #6 
On Mon, Oct 30, 2017 at 7:56 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 29/10/2017 18:59, H.J. Lu wrote:
>>
>>
>>>>>> 2017-10-29  Uros Bizjak  <ubizjak@gmail.com>
>>>>>>
>>>>>>      PR target/82725
>>>>>>      * g++.dg/pr82725.C: New test.
>
> The new testcase is failing for everybody in gnu++98 mode. I think you want
> to move it to the cpp0x directory and use the appropriate DejaGnu
> incantations.

Thanks for heads up. Fixed with:

2017-10-30  Uros Bizjak  <ubizjak@gmail.com>

* g++.dg/pr82725.C: Move to ...
* g++.dg/cpp0x/pr82725.C: ... here.  Add c++11 target directive.

Tested on x86_64-linux-gnu and committed.

Uros.

Index: g++.dg/cpp0x/pr82725.C
===================================================================
--- g++.dg/cpp0x/pr82725.C      (revision 254212)
+++ g++.dg/cpp0x/pr82725.C      (working copy)
@@ -1,4 +1,4 @@
-// { dg-do compile { target i?86-*-* x86_64-*-* } }
+// { dg-do compile { target { { i?86-*-* x86_64-*-* } && c++11 } } }
// { dg-require-effective-target pie }
// { dg-options "-O2 -fpie -mtls-direct-seg-refs" }
diff mbox series

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 254199)
+++ config/i386/i386.c	(working copy)
@@ -15079,10 +15079,16 @@  legitimate_pic_address_disp_p (rtx disp)
 	    break;
 	  op0 = XEXP (XEXP (disp, 0), 0);
 	  op1 = XEXP (XEXP (disp, 0), 1);
-	  if (!CONST_INT_P (op1)
-	      || INTVAL (op1) >= 16*1024*1024
+	  if (!CONST_INT_P (op1))
+	    break;
+	  if (GET_CODE (op0) == UNSPEC
+	      && (XINT (op0, 1) == UNSPEC_DTPOFF
+		  || XINT (op0, 1) == UNSPEC_NTPOFF)
+	      && trunc_int_for_mode (INTVAL (op1), SImode) == INTVAL (op1))
+	    return true;
+	  if (INTVAL (op1) >= 16*1024*1024
 	      || INTVAL (op1) < -16*1024*1024)
-            break;
+	    break;
 	  if (GET_CODE (op0) == LABEL_REF)
 	    return true;
 	  if (GET_CODE (op0) == CONST
Index: testsuite/g++.dg/pr82725.C
===================================================================
--- testsuite/g++.dg/pr82725.C	(nonexistent)
+++ testsuite/g++.dg/pr82725.C	(working copy)
@@ -0,0 +1,16 @@ 
+// { dg-do compile { target i?86-*-* x86_64-*-* } }
+// { dg-require-effective-target pie }
+// { dg-options "-O2 -fpie -mtls-direct-seg-refs" }
+
+struct string
+{
+  __SIZE_TYPE__ length;
+  const char *ptr;
+};
+
+string
+tempDir ()
+{
+  thread_local string cache;
+  return cache;
+}