On Wed, Apr 30, 2014 at 11:44 PM, Uros Bizjak <ubiz...@gmail.com> wrote:
> On Thu, May 1, 2014 at 6:42 AM, Wei Mi <w...@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.
>
Will add Changelog in the final patch.
>
> 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>).
>
If the generic code detects same pattern, it will not ICE but return
the original insn (See the code in try_split under the comment /*
Avoid infinite loop if any insn of the result matches the original
pattern. */). ix86_tls_descriptor_calls_expanded_in_cfun will be set
to true even if original insn is returned by try_split. That is how
pr58066-4 works.
> 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?
>
After we add add call into the pattern of tls_local_dynamic_base_32,
it is difficult for the pattern tls_local_dynamic_32_once to be
matched. I don't have a good way to rewrite tls_local_dynamic_32_once.
Do you have any idea?
(define_expand "tls_local_dynamic_base_32"
[(parallel
[(set (match_operand:SI 0 "register_operand")
(call:SI
(mem:QI (match_operand 2 "constant_call_address_operand"))
(const_int 0)))
(unspec:SI [(match_operand:SI 1 "register_operand")]
UNSPEC_TLS_LD_BASE)
(clobber (match_scratch:SI 3))
(clobber (match_scratch:SI 4))
(clobber (reg:CC FLAGS_REG))])]
> 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.
will do that.
Thanks,
Wei.
