Hi Sun,
It is target independent, all the targets need to get the right type of the
expression in order to find if the struct is returned in register and which
register contains the return value. This patch only fixed the problem that the
compiler mistakenly used struct type as return type, instead of field type, for
field access expression like "m.f2". Once the right type is found, the target
dependent part of the compiler will do the rest.
Thanks,
-Xiaohua
________________________________
From: Sun Chan <sun.c...@gmail.com>
To: Jian-Xin Lai <laij...@gmail.com>
Cc: Xiaohua Zhang <xiaohua_zh...@yahoo.com>; open64-devel@lists.sourceforge.net
Sent: Tue, June 28, 2011 3:50:15 PM
Subject: Re: [Open64-devel] code review request for bug 799
sorry for missing this. This change is not target independent is it?
Sun
On Tue, Jun 28, 2011 at 11:08 AM, Jian-Xin Lai <laij...@gmail.com> wrote:
> This patch looks fine to me. Please go ahead.
>
> 2011/6/25 Xiaohua Zhang <xiaohua_zh...@yahoo.com>:
>> Hi,
>>
>> Could a gatekeeper help to review the fix for bug 799?
>>
>> Thanks,
>> -Xiaohua
>>
>> The small test case is:
>>
>> #include <stdio.h>
>> int rval = 0;
>> typedef struct S { double i; } S;
>> S f(void);
>> typedef struct {
>> int f1;
>> struct S f2;
>> } S1;
>> S1 m;
>> struct S t = { 0.0 };
>> int main(void)
>> {
>> t.i = 12.345;
>> m.f2 = f(); // return value should be in xmm0
>> if (m.f2.i != t.i) {
>> rval = -1;
>> printf("Failed!\n");
>> }
>> return rval;
>> }
>> struct S f(void)
>> {
>> return t;
>> }
>>
>> The WHIRL tree for "m.f2 = f()" looks like:
>>
>> MCALL 126 <1,53,f> # flags 0x7e {line: 1/20}
>> MMLDID -1 <1,49,.preg_return_val> T<53,S,8>
>> MSTID 8 <1,54,m> T<55,.anonymous.1,8> <field_id:2> {line: 0/0}
>>
>> After the RETURN_VAL & MLDID/MSTID lowering:
>>
>> MCALL 126 <1,53,f> # flags 0x7e {line: 1/20}
>> I8I8LDID 1 <1,5,.preg_I8> T<5,.predef_I8,8> # $r1
>> I8STID 8 <1,54,m> T<5,.predef_I8,8> {line: 0/0}
>> F8F8LDID 17 <1,11,.preg_F8> T<11,.predef_F8,8> # $f0
>> F8STID 16 <1,54,m> T<11,.predef_F8,8> {line: 0/0}
>>
>> The return value (S) is returned by two register: $r1 and $f0.
>> By the ABI, the f()'s return value (type struct S) should be returned by one
>> floating register $f0.
>> The problem is in lower_return_mstid(), which takes MSTID's type (S1) as
>> return type, didn't consider the field (f2) it accessed. So it used the
>> wrong type (S1)to get return info, and the return value for S2 is by
>> register $r1 and $f0.
>>
>> The fix is to get the accessed field type as return type.
>>
>>
>>
>>
>>
>> ----------------------
>>
>> Index: be/com/wn_lower.cxx
>> ===================================================================
>> --- be/com/wn_lower.cxx (revision 3659)
>> +++ be/com/wn_lower.cxx (working copy)
>> @@ -7239,7 +7239,12 @@
>> ST *preg_st;
>> WN *n_rhs;
>> WN *wn = NULL; // init to prevent upward-exposed use
>> - RETURN_INFO return_info = Get_Return_Info(WN_ty(tree),
>> Complex_Not_Simulated
>> + TY_IDX ty_idx = WN_ty(tree);
>> +
>> + if (WN_field_id (tree) != 0)
>> + ty_idx = get_field_type (ty_idx, WN_field_id (tree));
>> +
>> + RETURN_INFO return_info = Get_Return_Info(ty_idx, Complex_Not_Simulated
>> #ifdef TARG_X8664
>> , last_call_ff2c_abi
>> #endif
>> @@ -7260,7 +7265,7 @@
>> #endif
>> WN *awn = WN_CreateLda(OPR_LDA, Pointer_Mtype, MTYPE_V,
>> WN_store_offset(tree),
>> - Make_Pointer_Type(WN_ty(tree)), WN_st_idx(tree));
>> + Make_Pointer_Type(ty_idx), WN_st_idx(tree));
>> awn = lower_expr(block, awn, actions);
>> WN *n_call = add_fake_parm(call, awn, WN_ty(awn));
>> WN_DELETE_FromBlock(block, call);
>> @@ -7277,9 +7282,9 @@
>> mtype = RETURN_INFO_mtype(return_info, i);
>> desc = mtype;
>> #ifdef KEY
>> - if (MTYPE_byte_size(mtype) > TY_size(WN_ty(tree)) &&
>> !MTYPE_float(mtype)){
>> + if (MTYPE_byte_size(mtype) > TY_size(ty_idx) && !MTYPE_float(mtype)){
>> desc = Mtype_AlignmentClass(1, MTYPE_type_class(mtype));
>> - while (MTYPE_byte_size(desc) < TY_size(WN_ty(tree)))
>> + while (MTYPE_byte_size(desc) < TY_size(ty_idx))
>> desc = Mtype_next_alignment(desc);
>> }
>> #endif
>> @@ -7287,7 +7292,7 @@
>>
>> #if defined(TARG_PPC32)
>> if (preg_st == Int_Preg) {
>> - UINT rem = TY_size(WN_ty(tree)) - i * 4;
>> + UINT rem = TY_size(ty_idx) - i * 4;
>> if (1 == rem) desc = MTYPE_I1;
>> if (2 == rem) desc = MTYPE_I2;
>> }
>>
>>
>>
>>
------------------------------------------------------------------------------
>> 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. Business sense. IT sense. Common sense..
>> http://p.sf.net/sfu/splunk-d2d-c1
>> _______________________________________________
>> Open64-devel mailing list
>> Open64-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/open64-devel
>>
>>
>
>
>
> --
> Regards,
> Lai Jian-Xin
>
> ------------------------------------------------------------------------------
> All of the data generated in your IT infrastructure is seriously valuable.
> Why? It contains a definitive record of 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-d2d-c2
> _______________________________________________
> Open64-devel mailing list
> Open64-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/open64-devel
>
------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of 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-d2d-c2
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
