Thank you very much.
I created a page on Open64 wiki for this kind of changes:
http://wiki.open64.net/index.php/WHIRLSpecUpdates
2012/2/9 Gautam Chakrabarti <gautam.c...@yahoo.com>:
> 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
>
>
--
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
