Thanks, David!
Your suggestion is exactly what I want. I have again a review on this patch
and the later bug916 patch, I think I need to re-orangize them to a
sequence of check-ins and still need more test cases to confirm.
My beginning example is a little bit misleading, sorry for that.
Regards
Gang
On Fri, Nov 18, 2011 at 7:35 AM, David Coakley <dcoak...@gmail.com> wrote:
> Thanks for the explanation. Here is a modified version of the test
> program that compiles without error using gcc and shows that 'P' is
> accepted as a modifier in the format string.
>
> void test(void)
> {
> char per_cpu__kernel_stack[100];
> void *ret__;
> asm volatile ("movq %%gs:%P1,%0"
> : "=q" (ret__)
> : "p" (&per_cpu__kernel_stack));
> }
>
> My best guess for the meaning of the 'P' modifier in this context is
> 'pointer-sized'. The gcc i386 machine description may shed some light
> on it.
>
> The only suggestion I have for your change is to separate the part for
> supporting the 'p' constraint from handling the 'P' modifier and make
> two check-ins, since they are really two separate problems.
>
> -David Coakey / AMD Open Source Compiler Engineering
>
> On Tue, Nov 15, 2011 at 4:09 PM, Gang Yu <yugang...@gmail.com> wrote:
> > Hi, David
> >
> > As for the "%P" format string, we directly get it from linux kernel
> source
> > code, a cpped snippet like below:
> >
> > # 6 "/fc/home/yug/kernel/opencc-build/arch/x86/include/asm/current.h" 2
> >
> > struct task_struct;
> >
> > extern __attribute__((section(".discard"), unused)) char
> > __pcpu_scope_current_task; extern __attribute__((section(".data.percpu"
> > ""))) __typeo\
> > f__(struct task_struct *) per_cpu__current_task;
> >
> > static inline __attribute__((always_inline)) struct task_struct
> > *get_current(void)
> > {
> > return ({ typeof(per_cpu__current_task) ret__; switch
> > (sizeof(per_cpu__current_task)) { case 1: asm("mov" "b ""%%""gs"":%P"
> > "1"",%0" : "=q" (\
> > ret__) : "p" (&per_cpu__current_task)); break; case 2: asm("mov" "w
> > ""%%""gs"":%P" "1"",%0" : "=r" (ret__) : "p" (&per_cpu__current_task));
> br\
> > eak; case 4: asm("mov" "l ""%%""gs"":%P" "1"",%0" : "=r" (ret__) : "p"
> > (&per_cpu__current_task)); break; case 8: asm("mov" "q ""%%""gs"":%P" "\
> > 1"",%0" : "=r" (ret__) : "p" (&per_cpu__current_task)); break; default:
> > __bad_percpu_size(); } ret__; });
> > }
> >
> > it is macro expansion result and not ready to read, so I provide the
> cpped
> > result.
> >
> > As for the gcc failed bug-cases,
> >
> > I think you use gcc O0 to test, for the fail reason, because gcc does not
> > localize the local char * per_cpu__kernel_stack to a memory location, so
> > &per_cpu__kernel_stack is reloaded as a register, that cause the
> assembler
> > assert. open64 does different for this case, so it does well.
> >
> > If you have further interest, please check out:
> >
> > https://svn.open64.net/svnroot/open64/regression_test
> >
> > runtest open64.exp=bug597,bug597-1,bug597-2,....,bug597-9
> >
> > Thanks
> >
> > Regards
> > Gang
> >
> >
> > On Wed, Nov 16, 2011 at 7:38 AM, David Coakley <dcoak...@gmail.com>
> wrote:
> >>
> >> Hi Gang,
> >>
> >> Your example is slightly more readable if you remove the extra quotes
> >> in the format string:
> >>
> >> void test(void)
> >> {
> >> char *per_cpu__kernel_stack;
> >> void *ret__;
> >> asm volatile ("movq %%gs:%P1,%0"
> >> : "=q" (ret__)
> >> : "p" (&per_cpu__kernel_stack));
> >> }
> >>
> >> You can see "%P" is part of the name "%P1", and 'P' is not documented
> >> as a valid modifier. It doesn't compile for me with gcc-4.1.2 or
> >> gcc-4.5.3:
> >>
> >> > gcc -c t.c
> >> t.c: Assembler messages:
> >> t.c:5: Error: bad memory operand `%rax'
> >>
> >> It does compile if I remove the 'P'. I understand the part of the bug
> >> related to adding support for the 'p' constraint. Are you sure the
> >> code related to capital 'P' in the format string is valid for x86?
> >>
> >> -David Coakley / AMD Open Source Compiler Engineering
> >>
> >>
> >> On Mon, Nov 14, 2011 at 5:40 PM, Gang Yu <yugang...@gmail.com> wrote:
> >> > Thank you, Sun
> >> >
> >> > As for the inline asm format string such as "%P", I google it but
> seems
> >> > very
> >> > few document on it. If people can provide relevant docs or links, it
> >> > will be
> >> > greatly appreciated, we can further improve it.
> >> >
> >> > Regards
> >> > Gang
> >> >
> >> >
> >> > On Tue, Nov 15, 2011 at 12:18 AM, Sun Chan <sun.c...@gmail.com>
> wrote:
> >> >>
> >> >> I am fine with this (although I suspect the use of strstr on %P may
> be
> >> >> not
> >> >> covering some error situations)
> >> >> Sun
> >> >>
> >> >> On Mon, Nov 14, 2011 at 8:35 PM, Gang Yu <yugang...@gmail.com>
> wrote:
> >> >>>
> >> >>> Hi,
> >> >>>
> >> >>> Could a gatekeeper please help review the fix for bug597,
> >> >>> constraint
> >> >>> <p> not implemented assertion? Thanks very much.
> >> >>>
> >> >>> Current status:
> >> >>>
> >> >>> open64 does not implement the "p" asm constraint for x86_64
> target,
> >> >>> assertion throws out:
> >> >>>
> >> >>> void test(void)
> >> >>> {
> >> >>> char *per_cpu__kernel_stack;
> >> >>> void *ret__;
> >> >>> asm volatile ("mov" "q ""%%""gs"":%P" "1"",%0"
> >> >>> : "=q" (ret__)
> >> >>> : "p" (&per_cpu__kernel_stack));
> >> >>> }
> >> >>>
> >> >>> ### Assertion failure at line 3323 of
> >> >>>
> >> >>>
> >> >>>
> /export/proj/linux_test/o64guru/base/trunk/objdir/osprey/../../osprey/be/cg/x8664/cgtarget.cxx:
> >> >>> ### Compiler Error in file asm-4.c during Code_Expansion phase:
> >> >>> ### ASM constraint <p> not supported
> >> >>>
> >> >>> The patch below
> >> >>>
> >> >>> --- a/osprey/be/cg/cgemit.cxx
> >> >>> +++ b/osprey/be/cg/cgemit.cxx
> >> >>> @@ -3526,13 +3526,17 @@ Modify_Asm_String (char* asm_string, UINT32
> >> >>> position, bool memory,
> >> >>> asm_string = Replace_Substring(asm_string, replace, name);
> >> >>> *name = tmp;
> >> >>> }
> >> >>> - if (strstr(asm_string, "%P")) {
> >> >>> - char replace[5];
> >> >>> - sprintf(replace, "%%P%d", position);
> >> >>> - // OSP_323, with "%P", we ignore the first character '$'
> >> >>> - asm_string = Replace_Substring(asm_string, replace, name+1);
> >> >>> - }
> >> >>> }
> >> >>> + if (strstr(asm_string, "%P")) {
> >> >>> + char replace[5];
> >> >>> + sprintf(replace, "%%P%d", position);
> >> >>> + // OSP_323, with "%P", we ignore the first character '$'
> >> >>> + if (*name == '$')
> >> >>> + asm_string = Replace_Substring(asm_string, replace,
> name+1);
> >> >>> + else
> >> >>> + asm_string = Replace_Substring(asm_string, replace, name);
> >> >>> + }
> >> >>> +
> >> >>> // Follow the zero dialect_number implementation as in
> >> >>> // gcc/final.c:output_asm_insn and handle {, } and | operators
> >> >>> if (strchr(asm_string, '{')) {
> >> >>> diff --git a/osprey/be/cg/x8664/cgtarget.cxx
> >> >>> b/osprey/be/cg/x8664/cgtarget.cxx
> >> >>> index e006d31..b1c0e58 100644
> >> >>> --- a/osprey/be/cg/x8664/cgtarget.cxx
> >> >>> +++ b/osprey/be/cg/x8664/cgtarget.cxx
> >> >>> @@ -106,6 +106,7 @@
> >> >>> #include "cg_loop.h"
> >> >>> #include "config_lno.h" // for LNO_Prefetch_Ahead
> >> >>> #include "erbe.h"
> >> >>> +#include "stblock.h" //for Base_Symbol_And_Offset_For_Addressing
> >> >>> @@ -3649,6 +3650,50 @@ CGTARG_TN_For_Asm_Operand (const char*
> >> >>> constraint,
> >> >>> else
> >> >>> ret_tn = (pref_tn ? pref_tn :
> >> >>> Build_RCLASS_TN(ISA_REGISTER_CLASS_float));
> >> >>> }
> >> >>> + else if (*constraint == 'p')
> >> >>> + {
> >> >>> + FmtAssert(load, (" there must be load expression for constraint
> >> >>> p\n"));
> >> >>> + if (load && WN_operator(load)==OPR_LDID &&
> >> >>> WN_class(load)==CLASS_PREG)
> >> >>> + {
> >> >>> + // immediate could have been put in preg by wopt
> >> >>> + load = Preg_Is_Rematerializable(WN_load_offset(load), NULL);
> >> >>> + }
> >> >>> + if (WN_operator(load) == OPR_INTCONST)
> >> >>> + {
> >> >>> + if (Is_Target_32bit() && (WN_const_val(load) > INT32_MAX ||
> >> >>> WN_const_val(load) < INT32_MIN)) {
> >> >>> + char c[200];
> >> >>> + sprintf(c,"%lld", WN_const_val(load));
> >> >>> + ErrMsgSrcpos(EC_Ill_Int_Oflow, WN_Get_Linenum(asm_wn),
> >> >>> + INT32_MIN,c,INT32_MAX);
> >> >>> + }
> >> >>> + ret_tn = Gen_Literal_TN(WN_const_val(load),
> >> >>> + MTYPE_bit_size(WN_rtype(load))/8);
> >> >>> + }
> >> >>> + else if ( WN_operator(load) == OPR_LDA &&
> >> >>> ST_sym_class(WN_st(load))
> >> >>> == CLASS_CONST)
> >> >>> + {
> >> >>> + ST * base;
> >> >>> + INT64 ofst;
> >> >>> + // Allocate the string to the rodata section
> >> >>> + Allocate_Object (WN_st(load));
> >> >>> + Base_Symbol_And_Offset (WN_st(load), &base, &ofst);
> >> >>> + ret_tn = Gen_Symbol_TN(base, ofst, 0);
> >> >>> + }
> >> >>> + else if ( WN_operator(load) == OPR_LDA &&
> >> >>> ST_sym_class(WN_st(load))
> >> >>> == CLASS_VAR)
> >> >>> + {
> >> >>> + ST * base;
> >> >>> + INT64 ofst;
> >> >>> + Base_Symbol_And_Offset_For_Addressing (WN_st(load),
> >> >>> WN_lda_offset(load), &base, &ofst);
> >> >>> + if (ofst == 0) { //no offset, using a symbol TN
> >> >>> + ret_tn = Gen_Symbol_TN(base, ofst, 0);
> >> >>> + } else { //has a offset, using a new TN to express the
> address
> >> >>> + ret_tn = (pref_tn ? pref_tn :
> >> >>> Build_TN_Of_Mtype(WN_rtype(load)));
> >> >>> + }
> >> >>> + }
> >> >>> + else //other cases
> >> >>> + {
> >> >>> + ret_tn = (pref_tn ? pref_tn :
> >> >>> Build_TN_Of_Mtype(WN_rtype(load)));
> >> >>> + }
> >> >>> + }
> >> >>> else
> >> >>> {
> >> >>> FmtAssert(FALSE, ("ASM constraint <%s> not supported",
> >> >>> constraint));
> >> >>>
> >> >>> Tests and validation :
> >> >>>
> >> >>> This patch has passed regression test bug597,bug597-1,...,bug597-9
> and
> >> >>> there are no such assertions thrown out on the whole
> kernel(2.6.32-9)
> >> >>> build.
> >> >>>
> >> >>>
> >> >>> Regards
> >> >>> Gang
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> ------------------------------------------------------------------------------
> >> >>> RSA(R) Conference 2012
> >> >>> Save $700 by Nov 18
> >> >>> Register now
> >> >>> http://p.sf.net/sfu/rsa-sfdev2dev1
> >> >>> _______________________________________________
> >> >>> Open64-devel mailing list
> >> >>> Open64-devel@lists.sourceforge.net
> >> >>> https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >>>
> >> >>
> >> >
> >> >
> >> >
> >> >
> ------------------------------------------------------------------------------
> >> > RSA(R) Conference 2012
> >> > Save $700 by Nov 18
> >> > Register now
> >> > http://p.sf.net/sfu/rsa-sfdev2dev1
> >> > _______________________________________________
> >> > Open64-devel mailing list
> >> > Open64-devel@lists.sourceforge.net
> >> > https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >
> >> >
> >
> >
>
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure
contains a definitive record of customers, application performance,
security threats, fraudulent activity, and more. Splunk takes this
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
