Looks good to me. Thank you very much.
2012/3/5 Gang Yu <yugang...@gmail.com>:
> Thanks for Jian-xin's review.
>
> Below is the improved patch, with no absolute%%ebx in the asm_string.
>
> index e75ebdc..54aa950 100644
> --- a/osprey/be/cg/cgemit.cxx
> +++ b/osprey/be/cg/cgemit.cxx
> @@ -3350,8 +3350,20 @@ Modify_Asm_String (char* asm_string, UINT32 position,
> bool memory,
>
> #endif
> if( base_ofst == 0 ){
> #if defined(TARG_X8664)
> - if( Is_Target_32bit() )
> - sprintf( buf, "%s", name );
> + // open64.net bug951.
> + // Format IA32 GOT symbol in Asm_String.
> + if( Is_Target_32bit() ) {
> + switch (TN_relocs(tn)) {
> + case TN_RELOC_IA32_GOT:
> + sprintf( buf,
> + "%s@GOT(%s)",
> + name,
> + int_reg_names[3][TN_register(Ebx_TN())-
> REGISTER_MIN]);
>
> + break;
> + default:
> + sprintf( buf, "%s", name);
> + }
> + }
> else
> sprintf( buf, "%s(%%rip)", name );
> #else
> @@ -3359,8 +3371,21 @@ Modify_Asm_String (char* asm_string, UINT32 position,
> bool memory,
>
> #endif
> } else
> #if defined(TARG_X8664)
> - if( Is_Target_32bit() )
> - sprintf( buf, "%s+%d", name, (int)base_ofst );
> + // open64.net bug951.
> + // Format IA32 GOTOFF symbol in Asm_String.
> + if( Is_Target_32bit() ) {
> + switch (TN_relocs(tn)) {
> + case TN_RELOC_IA32_GOTOFF:
> + sprintf( buf,
> + "%s@GOTOFF+%d(%s)",
> + name,
> + (int)base_ofst,
> + int_reg_names[3][TN_register(Ebx_TN())-
> REGISTER_MIN]);
> + break;
> + default:
>
> + sprintf( buf, "%s+%d", name, (int)base_ofst );
> + }
> + }
> else
> sprintf( buf, "%s+%d(%%rip)", name, (int)base_ofst );
> #else
> diff --git a/osprey/be/cg/x8664/cgtarget.cxx
> b/osprey/be/cg/x8664/cgtarget.cxx
> index 89f89d6..e78cbf9 100644
> --- a/osprey/be/cg/x8664/cgtarget.cxx
> +++ b/osprey/be/cg/x8664/cgtarget.cxx
> @@ -3908,7 +3908,15 @@ TN* CGTARG_Process_Asm_m_constraint( WN* load, void**
> offset, OPS* ops )
> if( WN_operator(load) == OPR_LDA ){
> OP* lda_op = OPS_last( ops );
> - asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) : OP_opnd( lda_op, 0
> );
> + // open64.net bug951. On finding the symbol TN, don't miss the cases
> of:
> + // TN :- ld32 GTN2(%rbx) (sym:base_sym +0)
> + // TN :- lea32 GTN2(%rbx) (sym:base_sym+base_ofst)
> + if (Is_Target_32bit() && Gen_PIC_Shared) {
> + asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) :
> + (OP_code(lda_op) == TOP_ldc32) ? OP_opnd( lda_op, 0) : OP_opnd(
> lda_op, 1);
> + } else {
> + asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) : OP_opnd( lda_op,
> 0 );
> + }
> OPS_Remove_Op( ops, lda_op );
> } else if( WN_operator(load) == OPR_ADD ){
> diff --git a/osprey/be/cg/x8664/exp_loadstore.cxx
> b/osprey/be/cg/x8664/exp_loadstore.cxx
> index 99bde2c..58cb61e 100644
> --- a/osprey/be/cg/x8664/exp_loadstore.cxx
> +++ b/osprey/be/cg/x8664/exp_loadstore.cxx
> @@ -1267,17 +1267,29 @@ Exp_Ldst (
> STB_section(base_sym) /* bug 10097 */)) ){
> FmtAssert(!ST_is_thread_local(base_sym),
> ("Exp_Ldst: thread-local storage NYI under PIC"));
> - TN* tmp = base_ofst == 0 ? tn : Build_TN_Like(tn);
> - Build_OP( TOP_ld32, tmp, Ebx_TN(),
> - Gen_Symbol_TN( base_sym, 0, TN_RELOC_IA32_GOT ),
> - &newops );
> +
> + // open64.net bug951.
> + // when base_ofst ! = 0, we generate
> + // TN :- lea32 GTN2(%rbx) (sym:base_sym+base_ofst)
> + // instead of
> + // tmp_TN :- ld32 GT2(%bx) (sym:base_sym + 0)
> + // TN :- lea32 tmp_TN base_ofst
> + // this is for consideration of facilitate restoring the register
> TN
> + // to symbol TN in ASM m constraint.
> + // And also this reduces a temp TN.
> +
> + if (base_ofst == 0)
> + Build_OP( TOP_ld32, tn, Ebx_TN(),
> + Gen_Symbol_TN( base_sym, base_ofst, TN_RELOC_IA32_GOT
> ),
> + &newops );
> + else
> + Build_OP( TOP_lea32, tn, Ebx_TN(),
> + Gen_Symbol_TN( base_sym, base_ofst,
> TN_RELOC_IA32_GOTOFF ),
> + &newops );
> // got address should not alias
> Set_OP_no_alias(OPS_last(&newops));
> PU_References_GOT = TRUE;
> - if( base_ofst != 0 ){
> - Build_OP( TOP_lea32, tn, tmp, Gen_Literal_TN(base_ofst, 4),
> &newops );
> - }
> } else {
> Build_OP( TOP_ldc32, tn,
> Gen_Symbol_TN( base_sym, base_ofst, TN_RELOC_NONE ),
>
>
> Regards
> Gang
>
>
> On Fri, Feb 24, 2012 at 2:30 PM, Gang Yu <yugang...@gmail.com> wrote:
>>
>> Hi,
>>
>> Could a gatekeeper please help review the fix for
>> bug951(http://bugs.open64.net/show_bug.cgi?id=951)?
>>
>> The cut-down bugcase:
>> typedef union {
>> } GenefxAccumulator;
>>
>> struct _GenefxState {
>> int length;
>> void *Aop[3];
>> int AopY;
>> GenefxAccumulator *Dacc;
>> GenefxAccumulator *Sacc;
>> };
>>
>> typedef struct _GenefxState GenefxState;
>> int main(void)
>> {
>> GenefxState *gfxs;
>> static const u32 preload[] = { 0xFF00FF00, 0x0000FF00 };
>> static const u32 mask[] = { 0x00FC00F8, 0x000000F8 };
>> static const u32 pm[] = { 0x01000004, 0x00000004 };
>> __asm__ __volatile__ (
>> "movq %3, %%mm7\n\t"
>> :
>> : "D" (gfxs->Aop[0]), "c" (gfxs->length), "S" (gfxs->Sacc),
>> "m" (*preload), "m" (*mask), "m" (*pm)
>> : "%eax", "%st", "memory");
>> return 0;
>> }
>>
>> opencc -m32 -fpic -S bug951.c
>> and opencc asserts:
>> ### Assertion failure at line 3377 of
>> /fc/proj/ctires/open64/o64guru/src/Thu/trunk/osprey/be/cg/cgemit.cxx:
>> ### Compiler Error in file bug951.c during Assembly phase:
>> ### ASM operand must be a register or a numeric constant
>>
>> Analysis:
>> The CGIR after code expansion phase for the asm statement is below:
>>
>> [ 58, 0] TN150 :- ld32 TN4(%rsp) (sym:gfxs+0) ; WN id:17 gfxs+0x0
>> [ 58, 0] TN5(%rdi) :- ld32 TN150 (0x8) ; WN id:23
>> [ 58, 0] TN8(%rcx) :- ld32 TN150 (0x0) ; WN id:24
>> [ 58, 0] TN6(%rsi) :- ld32 TN150 (0x1c) ; WN id:25
>> [ 58, 0] TN153 :- ld32 TN2(%rbx) (sym:.data.rel.ro.local+0) ; noalias WN
>> [ 58, 0] TN155 :- ld32 TN2(%rbx) (sym:.data.rel.ro.local+0) ; noalias WN
>> [ 58, 0] :- asm TN5(%rdi) TN8(%rcx) TN6(%rsi) TN2(%rbx) (0x10) (0x20) ;
>> volatile side_effects
>> -----------------------------------------------------------------------
>>
>> Obviously the asm stmt is unexpected, 0x10 as literal TN should not be "m"
>> constraint. "m" constraint in the ASM statement which is a constant address
>> should be symbol tn, not register tn, nor literal tn.
>>
>> The expected result should be
>> [ 22, 0] TN149 :- ld32 TN4(%rsp) (sym:gfxs+0) ; WN id:9 gfxs+0x0
>> [ 22, 0] TN5(%rdi) :- ld32 TN149 (0x4) ; WN id:10
>> [ 22, 0] TN8(%rcx) :- ld32 TN149 (0x0) ; WN id:11
>> [ 22, 0] TN6(%rsi) :- ld32 TN149 (0x18) ; WN id:12
>> [ 22, 0] :- asm TN5(%rdi) TN8(%rcx) TN6(%rsi) (sym:.data.rel.ro.local+0)
>> (sym:.data.rel.ro.local+16) (sym:.data.rel.ro.local+32) ; volatile
>> side_effects
>>
>> Investigate shows we miss the -m32 LDA_handling cases in
>> CGTARG_Asm_m_constraint.
>> asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) : OP_opnd( lda_op, 0 );
>> does not work for all cases.
>>
>> Suggested fix:
>>
>> use TN_RELOC_IA32_GOTOFF instead of TN_RELOC_IA32_GOT to load addresses
>> for symbols with offset, so to match the handling in ASM_m_constraint.
>>
>> below is the patch:
>>
>> index e75ebdc..ba246fd 100644
>> --- a/osprey/be/cg/cgemit.cxx
>> +++ b/osprey/be/cg/cgemit.cxx
>> @@ -3350,8 +3350,17 @@ Modify_Asm_String (char* asm_string, UINT32
>> position, bool memory,
>> #endif
>> if( base_ofst == 0 ){
>> #if defined(TARG_X8664)
>> - if( Is_Target_32bit() )
>> - sprintf( buf, "%s", name );
>> + // open64.net bug951.
>> + // Format IA32 GOT symbol in Asm_String.
>> + if( Is_Target_32bit() ) {
>> + switch (TN_relocs(tn)) {
>> + case TN_RELOC_IA32_GOT:
>> + sprintf( buf, "%s@GOT(%%ebx)", name );
>> + break;
>> + default:
>> + sprintf( buf, "%s", name);
>> + }
>> + }
>> else
>> sprintf( buf, "%s(%%rip)", name );
>> #else
>> @@ -3359,8 +3368,17 @@ Modify_Asm_String (char* asm_string, UINT32
>> position, bool memory,
>> #endif
>> } else
>> #if defined(TARG_X8664)
>> - if( Is_Target_32bit() )
>> - sprintf( buf, "%s+%d", name, (int)base_ofst );
>> + // open64.net bug951.
>> + // Format IA32 GOTOFF symbol in Asm_String.
>> + if( Is_Target_32bit() ) {
>> + switch (TN_relocs(tn)) {
>> + case TN_RELOC_IA32_GOTOFF:
>> + sprintf( buf, "%s@GOTOFF+%d(%%ebx)", name, (int)base_ofst);
>> + break;
>> + default:
>> + sprintf( buf, "%s+%d", name, (int)base_ofst );
>> + }
>> + }
>> else
>> sprintf( buf, "%s+%d(%%rip)", name, (int)base_ofst );
>> #else
>> diff --git a/osprey/be/cg/x8664/cgtarget.cxx
>> b/osprey/be/cg/x8664/cgtarget.cxx
>> index 89f89d6..e78cbf9 100644
>> --- a/osprey/be/cg/x8664/cgtarget.cxx
>> +++ b/osprey/be/cg/x8664/cgtarget.cxx
>> @@ -3908,7 +3908,15 @@ TN* CGTARG_Process_Asm_m_constraint( WN* load,
>> void** offset, OPS* ops )
>>
>> if( WN_operator(load) == OPR_LDA ){
>> OP* lda_op = OPS_last( ops );
>> - asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) : OP_opnd( lda_op,
>> 0 );
>> + // open64.net bug951. On finding the symbol TN, don't miss the cases
>> of:
>> + // TN :- ld32 GTN2(%rbx) (sym:base_sym +0)
>> + // TN :- lea32 GTN2(%rbx) (sym:base_sym+base_ofst)
>> + if (Is_Target_32bit() && Gen_PIC_Shared) {
>> + asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) :
>> + (OP_code(lda_op) == TOP_ldc32) ? OP_opnd( lda_op, 0) : OP_opnd(
>> lda_op, 1);
>> + } else {
>> + asm_opnd = OP_iadd(lda_op) ? OP_opnd( lda_op, 1 ) : OP_opnd(
>> lda_op, 0 );
>> + }
>> OPS_Remove_Op( ops, lda_op );
>>
>> } else if( WN_operator(load) == OPR_ADD ){
>> diff --git a/osprey/be/cg/x8664/exp_loadstore.cxx
>> b/osprey/be/cg/x8664/exp_loadstore.cxx
>> index 99bde2c..58cb61e 100644
>> --- a/osprey/be/cg/x8664/exp_loadstore.cxx
>> +++ b/osprey/be/cg/x8664/exp_loadstore.cxx
>> @@ -1267,17 +1267,29 @@ Exp_Ldst (
>> STB_section(base_sym) /* bug 10097 */)) ){
>> FmtAssert(!ST_is_thread_local(base_sym),
>> ("Exp_Ldst: thread-local storage NYI under PIC"));
>> - TN* tmp = base_ofst == 0 ? tn : Build_TN_Like(tn);
>> - Build_OP( TOP_ld32, tmp, Ebx_TN(),
>> - Gen_Symbol_TN( base_sym, 0, TN_RELOC_IA32_GOT ),
>> - &newops );
>> +
>> + // open64.net bug951.
>> + // when base_ofst ! = 0, we generate
>> + // TN :- lea32 GTN2(%rbx) (sym:base_sym+base_ofst)
>> + // instead of
>> + // tmp_TN :- ld32 GT2(%bx) (sym:base_sym + 0)
>> + // TN :- lea32 tmp_TN base_ofst
>> + // this is for consideration of facilitate restoring the
>> register TN
>> + // to symbol TN in ASM m constraint.
>> + // And also this reduces a temp TN.
>> +
>> + if (base_ofst == 0)
>> + Build_OP( TOP_ld32, tn, Ebx_TN(),
>> + Gen_Symbol_TN( base_sym, base_ofst,
>> TN_RELOC_IA32_GOT ),
>> + &newops );
>> + else
>> + Build_OP( TOP_lea32, tn, Ebx_TN(),
>> + Gen_Symbol_TN( base_sym, base_ofst,
>> TN_RELOC_IA32_GOTOFF ),
>> + &newops );
>> // got address should not alias
>> Set_OP_no_alias(OPS_last(&newops));
>> PU_References_GOT = TRUE;
>>
>> - if( base_ofst != 0 ){
>> - Build_OP( TOP_lea32, tn, tmp, Gen_Literal_TN(base_ofst, 4),
>> &newops );
>> - }
>> } else {
>> Build_OP( TOP_ldc32, tn,
>> Gen_Symbol_TN( base_sym, base_ofst, TN_RELOC_NONE ),
>>
>> TIA for the review.
>>
>> Regards
>> Gang
>
>
>
> ------------------------------------------------------------------------------
> Try before you buy = See our experts in action!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-dev2
> _______________________________________________
> Open64-devel mailing list
> Open64-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/open64-devel
>
--
Regards,
Lai Jian-Xin
------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
