The changes now look fine, please go ahead. I would also suggest to update the
WHIRL spec saying that for VLS the type size is -1.
-Gautam
________________________________
From: Jian-Xin Lai <laij...@gmail.com>
To: Gautam Chakrabarti <gautam.c...@yahoo.com>
Cc: open64-devel <open64-devel@lists.sourceforge.net>
Sent: Tuesday, February 7, 2012 12:47 AM
Subject: Re: [Open64-devel] code review request for bug 933 [WGEN]
(1):
I don't find any document about the size of VLS. But there are some
information in the debug info generated by GCC.
For a given type:
struct ST {
char x[size];
};
The debug info for the TYPE is:
<2><71>: Abbrev Number: 3 (DW_TAG_structure_type)
DW_AT_sibling : <8f>
DW_AT_name : ST
DW_AT_byte_size : 0xffffffff
DW_AT_decl_file : 1
DW_AT_decl_line : 4
From the output, I think we should set the size to -1.
There is another issue if the size is set to 0 in symtab verifier.
For the following case:
struct ST {
int f1;
int f2;
char x[size];
};
The field f2 has 4-byte offset, which is greater than the type size if
it's 0 and will cause an assertion in symtab verifier.
(2):
I made a new change to set the qualifiers before set the type_tree's IDX:
@@ -633,6 +632,21 @@
Set_TY_anonymous(ty);
Set_TY_etype (ty, Get_TY (gs_tree_type(type_tree)));
Set_TY_align (idx, TY_align(TY_etype(ty)));
+
+ // For GNU VLS (Variable length array in struct),
+ // the size and upper boundary is expression.
+ // If the TYPE_TY_IDX(type_tree) is not set, when
+ // expanding the TY's size, it will fall into a infinite
+ // recursion if the type_tree is referenced in the
+ // size expression. So we set the TYPE_TY_IDX here.
+ if (gs_type_readonly(type_tree))
+ Set_TY_is_const (idx);
+ if (gs_type_volatile(type_tree))
+ Set_TY_is_volatile (idx);
+ if (gs_type_restrict(type_tree))
+ Set_TY_is_restrict (idx);
+ TYPE_TY_IDX(type_tree) = idx;
+
// assumes 1 dimension
// nested arrays are treated as arrays of arrays
ARB_HANDLE arb = New_ARB ();
How do you think of the new change?
2012/2/7 Gautam Chakrabarti <gautam.c...@yahoo.com>:
> Hi Jian-Xin,
>
> Regarding (1): It's still not quite clear why tsize needs to be -1 for VLS.
> Is it just to match GCC's convention? I don't seem to find any place
> checking for this size of -1, except an assertion. If this special size is
> not really needed, then why not just use 0 for it? The size is still part of
> the WHIRL spec. So if it does need to be -1 for VLS, doesn't the spec need
> to be updated to reflect this? Could you explain why size needs to be -1?
>
> Regarding (2): Note that the TY_IDX can change after you set TYPE_TY_IDX if
> a type qualifier like volatile is applied to the type. Are type qualifiers
> applicable in this context? If no, then setting TYPE_TY_IDX in the new place
> should be safe.
>
> Other changes look fine.
>
> Thanks,
> Gautam
>
>
> From: Jian-Xin Lai <laij...@gmail.com>
> To: Gautam Chakrabarti <gautam.c...@yahoo.com>
> Cc: open64-devel <open64-devel@lists.sourceforge.net>
> Sent: Saturday, February 4, 2012 9:39 PM
> Subject: Re: [Open64-devel] code review request for bug 933 [WGEN]
>
> Hi Gautam,
>
> Thank you very much. Here is a new patch based on your comments.
>
> 1. The size of c99 VLA is 0 and GCC VLS is -1 in this patch now.
> In the debug info generated by GCC, the size of the GCC VLS is -1 and
> there is no size info for C99 VLA. So in the new patch, the size of
> C99 VLA is set to 0 according to the WHIRL spec and VLS to -1 to match
> GCC's behavior.
>
> 2. With VLS, the size of the TY is not an INT_CST expression. If the
> TYPE_TY_IDX(type_tree) is not set, when expanding the TY's size, it
> will fall into a infinite recursion if the type_tree is referenced in
> the size expression. I checked the assignment to the idx and found
> it's not changed between the new inserted line and the line of the
> original assignment. So it should be safe.
>
> 3. All others has been changed according to your comments.
>
> Could you please review again? Thank you very much.
>
> 2012/2/3 Gautam Chakrabarti <gautam.c...@yahoo.com>:
>> Hi Jian-Xin,
>>
>> Below are some questions/comments:
>>
>> Why did you change the tsize from 0 to -1? If this is indeed required,
>> then
>> this is an IR change, and the WHIRL symtab spec should be updated to
>> reflect
>> this. Also if this needs to change, then a global gatekeeper should
>> approve
>> this.
>>
>> In the following change from wgen_spin_symbol.cxx:
>>
>> @@ -633,6 +629,7 @@
>> Set_TY_anonymous(ty);
>> Set_TY_etype (ty, Get_TY (gs_tree_type(type_tree)));
>> Set_TY_align (idx, TY_align(TY_etype(ty)));
>> + TYPE_TY_IDX(type_tree) = idx;
>> // assumes 1 dimension
>> // nested arrays are treated as arrays of arrays
>> ARB_HANDLE arb = New_ARB ();
>>
>>
>> why did you set TYPE_TY_IDX as above? Because this is anyway set at the
>> end
>> of this function. Are you setting it because someone queries the IDX of
>> this
>> type before it's set at the end? Note that this idx at this point may not
>> be
>> the final IDX, and may be updated with other flags.
>>
>> Below are some minor comments:
>>
>> In WGEN_Compose_Address(), you could avoid creating multiple branches each
>> calling WN_CreateLda. Instead you could set up the offset based on "ofst"
>> and simplify the code, like you have done in WGEN_Create_Iload() and
>> elsewhere.
>>
>> Similarly WGEN_Create_Iload() and WGEN_Create_IStore() can be further
>> simplified to have a single call to WN_CreateIload/WN_CreateIStore by
>> first
>> setting up the address and offset.
>>
>> Thanks,
>> Gautam
>>
>>
>> From: Jian-Xin Lai <laij...@gmail.com>
>> To: open64-devel <open64-devel@lists.sourceforge.net>
>> Sent: Tuesday, January 17, 2012 3:18 AM
>> Subject: [Open64-devel] code review request for bug 933 [WGEN]
>>
>> Hi,
>>
>> Could a gatekeeper review the patch for bug #933? Thank you very much.
>>
>> This patch is used to support GCC extension - variable length array in
>> struct (VLA/VLS). There are 3 major kinds of changes in the patch:
>> 1. Set the static TY size of VLA/VLS from 0 to -1.
>> 2. Change all component offset in wgen_expr.cxx from WN_OFFSET to WN*.
>> For constant offset, the operator of WN* is INTCONST. For variable
>> offset, it's an integer expression.
>> 2. Wrap WN_CreateIload/WN_CreateIIStore/WN_CreateLdid/WN_CreateStid
>> for variable offset. If the offset is constant, the offset will be
>> attached onto the WN directly. Otherwise, an ADD node will be
>> generated for the address used by the load/store.
>>
>> So far there are still several limitation on this patch:
>> 1. don't support the copy between variable length object.
>> 2. don't support passing the variable length object by parameter or
>> return value. This one requires to patch the ABI part in BE and is
>> planed to fix later.
>>
>> I tested this patch with two methods:
>> 1. Tested the gcc regression. There is no regression found. The
>> following VLA/VLS test cases can pass now:
>> gcc.c-torture/compile/20050801-2.c
>> gcc.c-torture/compile/920428-4.c
>> gcc.c-torture/compile/920501-16.c
>> gcc.c-torture/execute/20040308-1.c
>> gcc.c-torture/execute/20040423-1.c
>> gcc.c-torture/execute/20041218-2.c
>> gcc.dg/pr19345.c
>> gcc.dg/pr30189.c
>> gcc.dg/vla-2.c
>>
>> The following cases still fails due to the limitation #2 mentioned above:
>> gcc.c-torture/compile/20010605-1.c
>> gcc.c-torture/compile/20030224-1.c
>> gcc.c-torture/execute/20020412-1.c
>> gcc.dg/compat/struct-by-value-22
>>
>> 2. Compile the test case without VLA/VLS by different WGEN (one is
>> original and the other is with this patch) and compare the .B file
>> dump. There is no difference found in the IR generated by the two
>> WGENs.
>>
>> --
>> Regards,
>> Lai Jian-Xin
>>
>>
>> ------------------------------------------------------------------------------
>> Keep Your Developer Skills Current with LearnDevNow!
>> 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-d2d
>> _______________________________________________
>> Open64-devel mailing list
>> Open64-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/open64-devel
>>
>>
>
>
>
> --
> Regards,
> Lai Jian-Xin
>
>
--
Regards,
Lai Jian-Xin------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
