This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, PR58066] preferred_stack_boundary update for tls expanded call


On Thu, May 1, 2014 at 6:42 AM, Wei Mi <wmi@google.com> wrote:
> Ping. Is pr58066-3.patch or pr58066-4.patch ok for trunk?

None of these patches have correct ChangeLog entries. Please follow
the rules, outlined in http://gcc.gnu.org/contribute.html (Submitting
Patches section), otherwise your patches will be simply ignored.

>>> I attached the patch which combined your two patches and the fix in
>>> legitimize_tls_address. I tried pr58066.c and c.i in ia32/x32/x86_64,
>>> the code looked fine. Do you think it is ok?
>>>
>>> Thanks,
>>> Wei.
>>
>> Either pr58066-3.patch or pr58066-4.patch looks good to me.

pr58066-4 patch is definitely not OK. I wonder, how it works at all,
since you can't split the insn to the same pattern. The generic code
detects this condition and forces ICE (IIRC: this is the reason for
UNSPEC_DIV_ALREADY_SPLIT tag in divmod<mode4_1>).

>From pr58066-3 patch:

-;; Local dynamic of a single variable is a lose.  Show combine how
-;; to convert that back to global dynamic.
-
-(define_insn_and_split "*tls_local_dynamic_32_once"
-  [(set (match_operand:SI 0 "register_operand" "=a")
-    (plus:SI
-     (unspec:SI [(match_operand:SI 1 "register_operand" "b")
-             (match_operand 2 "constant_call_address_operand" "z")]
-            UNSPEC_TLS_LD_BASE)
-     (const:SI (unspec:SI
-            [(match_operand 3 "tls_symbolic_operand")]
-            UNSPEC_DTPOFF))))
-   (clobber (match_scratch:SI 4 "=d"))
-   (clobber (match_scratch:SI 5 "=c"))
-   (clobber (reg:CC FLAGS_REG))]
-  ""
-  "#"
-  ""
-  [(parallel
-     [(set (match_dup 0)
-       (unspec:SI [(match_dup 1) (match_dup 3) (match_dup 2)]
-              UNSPEC_TLS_GD))
-      (clobber (match_dup 4))
-      (clobber (match_dup 5))
-      (clobber (reg:CC FLAGS_REG))])])

Why did you remove this splitter?

Please do not write:

+{
+  ix86_tls_descriptor_calls_expanded_in_cfun = true;
+})

but use a short form:

+  "ix86_tls_descriptor_calls_expanded_in_cfun = true;")

Please also add a testcase (from one of the previous mails):

--- testsuite/gcc.dg/pr58066.c (revision 0)
+++ testsuite/gcc.dg/pr58066.c (revision 0)

Put this test to gcc.target/i386 directory ...
@@ -0,0 +1,18 @@
+/* { dg-do compile { target {{ i?86-*-* x86_64-*-* } && { ! ia32 } } } } */

... to avoid target selector.

+/* { dg-options "-fPIC -O2" } */
+
+/* Check whether the stack frame starting addresses of tls expanded calls
+   in foo and goo are 16bytes aligned.  */
+static __thread char ccc1;
+void* foo()
+{
+ return &ccc1;
+}
+
+__thread char ccc2;
+void* goo()
+{
+ return &ccc2;
+}
+
+/* { dg-final { scan-assembler-times ".cfi_def_cfa_offset 16" 2 } } */

Please repost the complete patch with a proper ChangeLog.

Uros.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]