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
------------------------------------------------------------------------------
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
