[Open64-devel] [ABI, LOWER] Code review for bug #969, #970

Discussion:

Jian-Xin Lai

2012-07-31 08:15:45 UTC

Hi,

Could someone review the patch for open64 bug #969, #970? Thank you very much.

1. test case
#969 and #970 can be simplified to the following case:
struct I3 {
int a, b, c;
};
extern struct I3 foo();
void bar() {
struct I3 a;
a = foo();
}

The struct I1 can also be replaced by:
struct F3 {
float a, b, c;
};

2. IR dump
Open64 will allocate a temporary on stack for the return value. The IR
after MSTID lowering looks like:

MCALL 126 <1,51,foo> # flags 0x7e {line: 1/7}
I8I8LDID 1 <1,5,.preg_I8> T<5,.predef_I8,8> # $r1
I8STID 0 <2,2,_temp_.Mreturn.foo0> T<5,.predef_I8,8> {line: 0/0}
I8I8LDID 7 <1,5,.preg_I8> T<5,.predef_I8,8> # $r7
I8STID 8 <2,2,_temp_.Mreturn.foo0> T<5,.predef_I8,8> {line: 0/0}

[2]: _temp_.Mreturn.foo0 <2,2> Variable of type I1 (#53, KIND_STRUCT)
Address: 0(_temp_.Mreturn.foo0<2,2>) Alignment: 8 bytes
location: file (null), line 0
Flags: 0x00400000 temp, XLOCAL
Sclass: AUTO

[53]: I3 : (f: 0x1000 content_seen) size 12 M: STRUCT
0 a .predef_I4 (#4) align 4
fl:0x0000
4 b .predef_I4 (#4) align 4
fl:0x0000
8 c .predef_I4 (#4) align 4
fl:0x0001 last_field

The size of _temp_.Mreturn.foo0 is only 12 byte. The I8STID will
overwrite some other variables on stack. For float struct F3, two
F8STID will be generated.

3. Patch
Index: osprey/be/com/wn_lower.cxx
===================================================================
--- osprey/be/com/wn_lower.cxx (revision 3998)
+++ osprey/be/com/wn_lower.cxx (working copy)
@@ -7299,6 +7299,7 @@
}
else { // return via 1 or more return registers
TYPE_ID mtype, desc;
+ WN_OFFSET st_ofst = WN_store_offset(tree);
for (INT32 i = 0; i < RETURN_INFO_count(return_info); i++) {
if (i != 0)
WN_INSERT_BlockLast (block, wn); // insert the last STID created
@@ -7334,8 +7335,9 @@
}
#endif
wn = WN_CreateStid(OPR_STID, MTYPE_V, desc,
- WN_store_offset(tree)+i*MTYPE_byte_size(mtype),
+ st_ofst,
WN_st_idx(tree), Be_Type_Tbl(desc), n_rhs);
+ st_ofst += MTYPE_byte_size(mtype);
wn = lower_store (block, wn, actions);
WN_Set_Linenum(wn, current_srcpos);
}
@@ -7394,6 +7396,7 @@
}
else { // return via 1 or more return registers
WN *base_expr;
+ WN_OFFSET st_ofst = WN_store_offset(tree);
for (INT32 i = 0; i < RETURN_INFO_count(return_info); i++) {
if (i != 0)
WN_INSERT_BlockLast (block, wn); // insert the last STID created
@@ -7406,8 +7409,9 @@
// OSP-438: The type of Istore node should be a pointer to mtype.
// (Previously, we just use Be_Type_Tbl(mtype) as the type of Istore).
wn = WN_CreateIstore(OPR_ISTORE, MTYPE_V, mtype,
- WN_store_offset(tree)+i*MTYPE_byte_size(mtype),
+ st_ofst,
Make_Pointer_Type(Be_Type_Tbl(mtype),
FALSE), n_rhs, base_expr);
+ st_ofst += MTYPE_byte_size(mtype);
WN_Set_Linenum(wn, WN_Get_Linenum(tree));
wn = lower_store (block, wn, actions);
WN_Set_Linenum (wn, WN_Get_Linenum(tree));
@@ -12285,6 +12289,7 @@
("expected RETURN_VAL kid not type M"));

algn = TY_align(ty_idx);
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12312,15 +12317,14 @@
#endif
#if defined(TARG_PPC32)
n_rhs = WN_CreateLdid (OPR_LDID, mtype, desc,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
WN_st_idx(o_rhs), ty_idx_used);
#else
n_rhs = WN_CreateLdid (OPR_LDID, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
WN_st_idx(o_rhs), ty_idx_used);
#endif
+ ld_ofst += MTYPE_byte_size(mtype);
wn = WN_CreateStid(OPR_STID, MTYPE_V, mtype, preg, preg_st,
Be_Type_Tbl(mtype), n_rhs);
WN_Set_Linenum(wn, current_srcpos); // Bug 1268
@@ -12332,6 +12336,7 @@
("expected RETURN_VAL kid not type M"));

algn = TY_align(ty_idx);
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12340,10 +12345,10 @@
n_rhs = WN_kid0(o_rhs);
else n_rhs = WN_COPY_Tree(WN_kid0(o_rhs));
n_rhs = WN_CreateIload(OPR_ILOAD, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
ty_idx_used,
Make_Pointer_Type(ty_idx_used), n_rhs);
+ ld_ofst += MTYPE_byte_size(mtype);
n_rhs = lower_expr(block, n_rhs, actions);
preg = RETURN_INFO_preg (return_info, i);
preg_st = Standard_Preg_For_Mtype(mtype);
@@ -12357,6 +12362,7 @@
Is_True(WN_operator(WN_kid1(o_rhs)) == OPR_INTCONST,
("expected RETURN_VAL's MLOAD kid to be of constant size"));
algn = TY_align(TY_pointed(WN_load_addr_ty(o_rhs)));
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12365,10 +12371,10 @@
n_rhs = WN_kid0(o_rhs);
else n_rhs = WN_COPY_Tree(WN_kid0(o_rhs));
n_rhs = WN_CreateIload(OPR_ILOAD, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
ty_idx_used,
Make_Pointer_Type(ty_idx_used), n_rhs);
+ ld_ofst += MTYPE_byte_size(mtype);
n_rhs = lower_expr(block, n_rhs, actions);
preg = RETURN_INFO_preg (return_info, i);
preg_st = Standard_Preg_For_Mtype(mtype);
Index: osprey/common/com/x8664/targ_sim.cxx
===================================================================
--- osprey/common/com/x8664/targ_sim.cxx (revision 3998)
+++ osprey/common/com/x8664/targ_sim.cxx (working copy)
@@ -673,13 +673,13 @@
}
else {
if (classes[0] == X86_64_SSE_CLASS) {
- info.mtype[0] = SIM_INFO.dbl_type;
+ info.mtype[0] = (size > 4) ? MTYPE_F8 : MTYPE_F4;
info.preg[0] = PR_first_reg(SIM_INFO.dbl_results);
next_float_return_num = PR_last_reg(SIM_INFO.dbl_results);
next_int_return_num = PR_first_reg(SIM_INFO.int_results);
}
else {
- info.mtype[0] = SIM_INFO.int_type;
+ info.mtype[0] = (size > 4) ? MTYPE_I8 : MTYPE_I4;
info.preg[0] = PR_first_reg(SIM_INFO.int_results);
next_float_return_num = PR_first_reg(SIM_INFO.dbl_results);
next_int_return_num = PR_last_reg(SIM_INFO.int_results);
@@ -687,11 +687,11 @@

if (n > 1) {
if (classes[1] == X86_64_SSE_CLASS) {
- info.mtype[1] = SIM_INFO.dbl_type;
+ info.mtype[1] = (size > 12) ? MTYPE_F8 : MTYPE_F4;
info.preg[1] = next_float_return_num;
}
else {
- info.mtype[1] = SIM_INFO.int_type;
+ info.mtype[1] = (size > 12) ? MTYPE_I8 : MTYPE_I4;
info.preg[1] = next_int_return_num;
}
}

--
Regards,
Lai Jian-Xin

Sun Chan

2012-07-31 08:46:00 UTC

Permalink

I think this problem is target dependent. For x86, is 12 bytes wrong?
I remember it is correct for SGI (which does not matter anymore), but
the point really is, this is ABI dependent
Sun

Post by Jian-Xin Lai
Hi,
Could someone review the patch for open64 bug #969, #970? Thank you very much.
1. test case
struct I3 {
int a, b, c;
};
extern struct I3 foo();
void bar() {
struct I3 a;
a = foo();
}
struct F3 {
float a, b, c;
};
2. IR dump
Open64 will allocate a temporary on stack for the return value. The IR
MCALL 126 <1,51,foo> # flags 0x7e {line: 1/7}
I8I8LDID 1 <1,5,.preg_I8> T<5,.predef_I8,8> # $r1
I8STID 0 <2,2,_temp_.Mreturn.foo0> T<5,.predef_I8,8> {line: 0/0}
I8I8LDID 7 <1,5,.preg_I8> T<5,.predef_I8,8> # $r7
I8STID 8 <2,2,_temp_.Mreturn.foo0> T<5,.predef_I8,8> {line: 0/0}
[2]: _temp_.Mreturn.foo0 <2,2> Variable of type I1 (#53, KIND_STRUCT)
Address: 0(_temp_.Mreturn.foo0<2,2>) Alignment: 8 bytes
location: file (null), line 0
Flags: 0x00400000 temp, XLOCAL
Sclass: AUTO
[53]: I3 : (f: 0x1000 content_seen) size 12 M: STRUCT
0 a .predef_I4 (#4) align 4
fl:0x0000
4 b .predef_I4 (#4) align 4
fl:0x0000
8 c .predef_I4 (#4) align 4
fl:0x0001 last_field
The size of _temp_.Mreturn.foo0 is only 12 byte. The I8STID will
overwrite some other variables on stack. For float struct F3, two
F8STID will be generated.
3. Patch
Index: osprey/be/com/wn_lower.cxx
===================================================================
--- osprey/be/com/wn_lower.cxx (revision 3998)
+++ osprey/be/com/wn_lower.cxx (working copy)
@@ -7299,6 +7299,7 @@
}
else { // return via 1 or more return registers
TYPE_ID mtype, desc;
+ WN_OFFSET st_ofst = WN_store_offset(tree);
for (INT32 i = 0; i < RETURN_INFO_count(return_info); i++) {
if (i != 0)
WN_INSERT_BlockLast (block, wn); // insert the last STID created
@@ -7334,8 +7335,9 @@
}
#endif
wn = WN_CreateStid(OPR_STID, MTYPE_V, desc,
- WN_store_offset(tree)+i*MTYPE_byte_size(mtype),
+ st_ofst,
WN_st_idx(tree), Be_Type_Tbl(desc), n_rhs);
+ st_ofst += MTYPE_byte_size(mtype);
wn = lower_store (block, wn, actions);
WN_Set_Linenum(wn, current_srcpos);
}
@@ -7394,6 +7396,7 @@
}
else { // return via 1 or more return registers
WN *base_expr;
+ WN_OFFSET st_ofst = WN_store_offset(tree);
for (INT32 i = 0; i < RETURN_INFO_count(return_info); i++) {
if (i != 0)
WN_INSERT_BlockLast (block, wn); // insert the last STID created
@@ -7406,8 +7409,9 @@
// OSP-438: The type of Istore node should be a pointer to mtype.
// (Previously, we just use Be_Type_Tbl(mtype) as the type of Istore).
wn = WN_CreateIstore(OPR_ISTORE, MTYPE_V, mtype,
- WN_store_offset(tree)+i*MTYPE_byte_size(mtype),
+ st_ofst,
Make_Pointer_Type(Be_Type_Tbl(mtype),
FALSE), n_rhs, base_expr);
+ st_ofst += MTYPE_byte_size(mtype);
WN_Set_Linenum(wn, WN_Get_Linenum(tree));
wn = lower_store (block, wn, actions);
WN_Set_Linenum (wn, WN_Get_Linenum(tree));
@@ -12285,6 +12289,7 @@
("expected RETURN_VAL kid not type M"));
algn = TY_align(ty_idx);
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12312,15 +12317,14 @@
#endif
#if defined(TARG_PPC32)
n_rhs = WN_CreateLdid (OPR_LDID, mtype, desc,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
WN_st_idx(o_rhs), ty_idx_used);
#else
n_rhs = WN_CreateLdid (OPR_LDID, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
WN_st_idx(o_rhs), ty_idx_used);
#endif
+ ld_ofst += MTYPE_byte_size(mtype);
wn = WN_CreateStid(OPR_STID, MTYPE_V, mtype, preg, preg_st,
Be_Type_Tbl(mtype), n_rhs);
WN_Set_Linenum(wn, current_srcpos); // Bug 1268
@@ -12332,6 +12336,7 @@
("expected RETURN_VAL kid not type M"));
algn = TY_align(ty_idx);
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12340,10 +12345,10 @@
n_rhs = WN_kid0(o_rhs);
else n_rhs = WN_COPY_Tree(WN_kid0(o_rhs));
n_rhs = WN_CreateIload(OPR_ILOAD, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
ty_idx_used,
Make_Pointer_Type(ty_idx_used), n_rhs);
+ ld_ofst += MTYPE_byte_size(mtype);
n_rhs = lower_expr(block, n_rhs, actions);
preg = RETURN_INFO_preg (return_info, i);
preg_st = Standard_Preg_For_Mtype(mtype);
@@ -12357,6 +12362,7 @@
Is_True(WN_operator(WN_kid1(o_rhs)) == OPR_INTCONST,
("expected RETURN_VAL's MLOAD kid to be of constant size"));
algn = TY_align(TY_pointed(WN_load_addr_ty(o_rhs)));
+ WN_OFFSET ld_ofst = WN_load_offset(o_rhs);
for (i = 0; i < RETURN_INFO_count(return_info); i++) {
mtype = RETURN_INFO_mtype(return_info, i);
ty_idx_used = Be_Type_Tbl(mtype);
@@ -12365,10 +12371,10 @@
n_rhs = WN_kid0(o_rhs);
else n_rhs = WN_COPY_Tree(WN_kid0(o_rhs));
n_rhs = WN_CreateIload(OPR_ILOAD, mtype, mtype,
- WN_load_offset(o_rhs)
- + i * MTYPE_byte_size(mtype),
+ ld_ofst,
ty_idx_used,
Make_Pointer_Type(ty_idx_used), n_rhs);
+ ld_ofst += MTYPE_byte_size(mtype);
n_rhs = lower_expr(block, n_rhs, actions);
preg = RETURN_INFO_preg (return_info, i);
preg_st = Standard_Preg_For_Mtype(mtype);
Index: osprey/common/com/x8664/targ_sim.cxx
===================================================================
--- osprey/common/com/x8664/targ_sim.cxx (revision 3998)
+++ osprey/common/com/x8664/targ_sim.cxx (working copy)
@@ -673,13 +673,13 @@
}
else {
if (classes[0] == X86_64_SSE_CLASS) {
- info.mtype[0] = SIM_INFO.dbl_type;
+ info.mtype[0] = (size > 4) ? MTYPE_F8 : MTYPE_F4;
info.preg[0] = PR_first_reg(SIM_INFO.dbl_results);
next_float_return_num = PR_last_reg(SIM_INFO.dbl_results);
next_int_return_num = PR_first_reg(SIM_INFO.int_results);
}
else {
- info.mtype[0] = SIM_INFO.int_type;
+ info.mtype[0] = (size > 4) ? MTYPE_I8 : MTYPE_I4;
info.preg[0] = PR_first_reg(SIM_INFO.int_results);
next_float_return_num = PR_first_reg(SIM_INFO.dbl_results);
next_int_return_num = PR_last_reg(SIM_INFO.int_results);
@@ -687,11 +687,11 @@
if (n > 1) {
if (classes[1] == X86_64_SSE_CLASS) {
- info.mtype[1] = SIM_INFO.dbl_type;
+ info.mtype[1] = (size > 12) ? MTYPE_F8 : MTYPE_F4;
info.preg[1] = next_float_return_num;
}
else {
- info.mtype[1] = SIM_INFO.int_type;
+ info.mtype[1] = (size > 12) ? MTYPE_I8 : MTYPE_I4;
info.preg[1] = next_int_return_num;
}
}
--
Regards,
Lai Jian-Xin
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Open64-devel mailing list
https://lists.sourceforge.net/lists/listinfo/open64-devel

Jian-Xin Lai

2012-07-31 10:05:01 UTC

Permalink

On x86_64, the aggregate shown in the test case is partitioned into
two eightbyte pieces. Both of them are classified to INTEGER class and
returned by two 64-bit registers (RAX and RDX).
If the size of the aggregate is larger than 12, both of the two parts
should be MTYPE_I8 and use I8 for LDID/STID. Otherwise, the first part
is MTYE_I8 and the second part should be MTYPE_I4.

Post by Sun Chan
I think this problem is target dependent. For x86, is 12 bytes wrong?
I remember it is correct for SGI (which does not matter anymore), but
the point really is, this is ABI dependent
Sun

--
Regards,
Lai Jian-Xin

Sun Chan

2012-07-31 10:43:12 UTC

Permalink

And IA64?
Sun

Post by Jian-Xin Lai
On x86_64, the aggregate shown in the test case is partitioned into
two eightbyte pieces. Both of them are classified to INTEGER class and
returned by two 64-bit registers (RAX and RDX).
If the size of the aggregate is larger than 12, both of the two parts
should be MTYPE_I8 and use I8 for LDID/STID. Otherwise, the first part
is MTYE_I8 and the second part should be MTYPE_I4.

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-07-31 10:56:57 UTC

Permalink

IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Sun Chan

2012-07-31 11:17:59 UTC

Permalink

Mips used to pass float in integer registers, I don't know if they
continue to doing that. Longson people?
Sun

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-07-31 14:37:36 UTC

Permalink

I plan to change my patch. I think the change in targ_sim.cxx isn't necessary since not only X86_64 has this issue. It looks like it's better to fix this issue in the lowering phase. For the last piece of return value, we should't use the mtype from the return info. A new mtype should be calculated based on the size of the last piece.
I'll send a new patch later.

Regards,
Jian-Xin

在 2012-7-31，19:17，Sun Chan <***@gmail.com> 写道：

Mips used to pass float in integer registers, I don't know if they
continue to doing that. Longson people?
Sun

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-08-01 07:55:04 UTC

Permalink

Hi Sun,

Could you please review the new patch? Thank you very much.
A new function adjust_type_for_return_info is added to adjust the
rtype and desc type according to the size left for copy.

Post by Jian-Xin Lai
I plan to change my patch. I think the change in targ_sim.cxx isn't necessary since not only X86_64 has this issue. It looks like it's better to fix this issue in the lowering phase. For the last piece of return value, we should't use the mtype from the return info. A new mtype should be calculated based on the size of the last piece.
I'll send a new patch later.
Regards,
Jian-Xin
Mips used to pass float in integer registers, I don't know if they
continue to doing that. Longson people?
Sun

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-08-17 10:55:46 UTC

Permalink

Hi,

Could someone review this patch? Thank you very much.

Post by Jian-Xin Lai
Hi Sun,
Could you please review the new patch? Thank you very much.
A new function adjust_type_for_return_info is added to adjust the
rtype and desc type according to the size left for copy.

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Sun Chan

2012-08-17 23:27:31 UTC

Permalink

your changed touched upon ppc target, did you check with ppc people
and get their ok?
Sun

Post by Jian-Xin Lai
Hi,
Could someone review this patch? Thank you very much.

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-08-21 06:33:50 UTC

Permalink

Hi Prof. Dong,

Since your team maintains the PPC port, could you please review my
change? Thank you very much.

Post by Sun Chan
your changed touched upon ppc target, did you check with ppc people
and get their ok?
Sun

Post by Jian-Xin Lai
Hi,
Could someone review this patch? Thank you very much.

Post by Jian-Xin Lai
IA-64 has the similar problem.

Post by Sun Chan
And IA64?
Sun

--
Regards,
Lai Jian-Xin

Jian-Xin Lai

2012-08-30 05:01:00 UTC

Permalink

Actually, the PPC part is also trying to do the samething as I do in
this patch. So I think my patch is also safe on PPC32 target and it's
more generic.

Post by Sun Chan
your changed touched upon ppc target, did you check with ppc people
and get their ok?
Sun

Post by Jian-Xin Lai
Hi,
Could someone review this patch? Thank you very much.