diff mbox

[Google/gcc-4_9,target/x86_64] PR 63538

Message ID CAAs8HmwUsxkmbyYCKMhjw3xhJS7nyBhtTfBUiKF6swFG=bmcnA@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Oct. 20, 2014, 8:44 p.m. UTC
On Mon, Oct 20, 2014 at 12:59 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Oct 20, 2014 at 11:59 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>> On Mon, Oct 20, 2014 at 10:51 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Mon, Oct 20, 2014 at 10:46 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>> On Mon, Oct 20, 2014 at 10:42 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> Why removing the tree_code check?
>>>>
>>>> The actual problem happens because STRING_CSTs (end up in .lrodata)
>>>> are not set a far address as they dont match the VAR_DECL check here.
>>>> Futher,  "ix86_in_large_data_p" call has the TREE_CODE check to do the
>>>> right thing so this seems unnecessary & buggy here.
>>>
>>> I think he is asking because TREE_STATIC (decl) || DECL_EXTERNAL
>>> (decl) might be an issue for STRING_CSTs.
>>
>> TREE_STATIC is true for STRING_CSTs and DECL_EXTERNAL false, that looks ok.
>
> The values for STRING_CST make sense, but it is not documented in
> tree.h for use with STRING_CST. Maybe do this:
>
>  if (((TREE_CODE (decl) == VAR_DECL&& (TREE_STATIC (decl) ||
> DECL_EXTERNAL (decl))
>        ||TREE_CODE (decl) == STRING_CST)
>       && ix86_in_large_data_p (decl))
>
> which can be simplified to:
>
>  if ((TREE_CODE (decl) == VAR_DECL && is_global_var (decl) ||TREE_CODE
> (decl) == STRING_CST)
>   && ix86_in_large_data_p (decl))

Patch updated.

Thanks
Sri

>      ...
>
> David
>>
>> Thanks
>> Sri
>>
>>>
>>> Thanks,
>>> Andrew
>>>
>>>
>>>>
>>>> Thanks
>>>> Sri
>>>>
>>>>>
>>>>> David
>>>>>
>>>>> On Mon, Oct 20, 2014 at 10:35 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>>    This patch is under review for trunk GCC :
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01638.html.
>>>>>>
>>>>>>     In the mean time, is this ok for google/gcc-4_9 branch?  Without
>>>>>> this, -mcmodel=medium is unusable if .lrodata goes beyond the 2G
>>>>>> boundary.
>>>>>>
>>>>>> Thanks
>>>>>> Sri

Comments

Xinliang David Li Oct. 20, 2014, 8:49 p.m. UTC | #1
ok (for google branch).

David

On Mon, Oct 20, 2014 at 1:44 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Mon, Oct 20, 2014 at 12:59 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Mon, Oct 20, 2014 at 11:59 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> On Mon, Oct 20, 2014 at 10:51 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>>>> On Mon, Oct 20, 2014 at 10:46 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>> On Mon, Oct 20, 2014 at 10:42 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> Why removing the tree_code check?
>>>>>
>>>>> The actual problem happens because STRING_CSTs (end up in .lrodata)
>>>>> are not set a far address as they dont match the VAR_DECL check here.
>>>>> Futher,  "ix86_in_large_data_p" call has the TREE_CODE check to do the
>>>>> right thing so this seems unnecessary & buggy here.
>>>>
>>>> I think he is asking because TREE_STATIC (decl) || DECL_EXTERNAL
>>>> (decl) might be an issue for STRING_CSTs.
>>>
>>> TREE_STATIC is true for STRING_CSTs and DECL_EXTERNAL false, that looks ok.
>>
>> The values for STRING_CST make sense, but it is not documented in
>> tree.h for use with STRING_CST. Maybe do this:
>>
>>  if (((TREE_CODE (decl) == VAR_DECL&& (TREE_STATIC (decl) ||
>> DECL_EXTERNAL (decl))
>>        ||TREE_CODE (decl) == STRING_CST)
>>       && ix86_in_large_data_p (decl))
>>
>> which can be simplified to:
>>
>>  if ((TREE_CODE (decl) == VAR_DECL && is_global_var (decl) ||TREE_CODE
>> (decl) == STRING_CST)
>>   && ix86_in_large_data_p (decl))
>
> Patch updated.
>
> Thanks
> Sri
>
>>      ...
>>
>> David
>>>
>>> Thanks
>>> Sri
>>>
>>>>
>>>> Thanks,
>>>> Andrew
>>>>
>>>>
>>>>>
>>>>> Thanks
>>>>> Sri
>>>>>
>>>>>>
>>>>>> David
>>>>>>
>>>>>> On Mon, Oct 20, 2014 at 10:35 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>    This patch is under review for trunk GCC :
>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01638.html.
>>>>>>>
>>>>>>>     In the mean time, is this ok for google/gcc-4_9 branch?  Without
>>>>>>> this, -mcmodel=medium is unusable if .lrodata goes beyond the 2G
>>>>>>> boundary.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Sri
diff mbox

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 216287)
+++ config/i386/i386.c	(working copy)
@@ -41331,8 +41331,8 @@  ix86_encode_section_info (tree decl, rtx rtl, int
 {
   default_encode_section_info (decl, rtl, first);
 
-  if (TREE_CODE (decl) == VAR_DECL
-      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))
+  if (((TREE_CODE (decl) == VAR_DECL && is_global_var (decl))
+       || TREE_CODE(decl) == STRING_CST)
       && ix86_in_large_data_p (decl))
     SYMBOL_REF_FLAGS (XEXP (rtl, 0)) |= SYMBOL_FLAG_FAR_ADDR;
 }
Index: testsuite/gcc.dg/pr63538.c
===================================================================
--- testsuite/gcc.dg/pr63538.c	(revision 0)
+++ testsuite/gcc.dg/pr63538.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* PR63538 is about not using 64-bit addresses for .lrodata accesses when it
+   involves STRING_CSTs.  */
+/* { dg-do compile { target x86_64-*-* } } */
+/* { dg-options "-O2 -mcmodel=medium -mlarge-data-threshold=0" { target x86_64-*-* } } */
+
+#include <stdio.h>
+
+const char *str = "Hello World";
+
+int main() {
+ printf("str = %p %s\n",str, str);
+ return 0;
+}
+/* { dg-final { scan-assembler-not "movl" } } */