This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: fix incorrect SRA transformation on non-integral VIEW_CONVERT argument


On Wed, Apr 25, 2012 at 3:37 PM, Olivier Hainque <hainque@adacore.com> wrote:
> Hello,
>
> For the ?"PA(1).Z := 44;" assignment in the attached Ada
> testcase, we observe the gcc 4.5 SRA pass performing an
> invalid transformation, turning:
>
> ?struct {
> ? ?system__pack_48__bits_48 OBJ;
> ?} D.1432;
>
> ?D.1432.OBJ = D.1435;
> ?T1b.F = VIEW_CONVERT_EXPR<struct pt__point>(D.1432);
>
> into:
>
> ?SR.12_17 = D.1435_3;
> ?T1b.F = VIEW_CONVERT_EXPR<struct pt__point>(SR.12_17);
>
> where we have
>
> ? ?<var_decl D.1432
> ? ? type <record_type 0x7ffff7fb72a0 BLK
> ? ? ? ?size <integer_cst 0x7ffff7fac960 constant 48>
>
> and
>
> ? ?<var_decl SR.12
> ? ? type <integer_type system__pack_48__bits_48
> ? ? ? ?size <integer_cst 0x7ffff7ecd870 64>
>
> ? ? ? ?type <integer_type system__pack_48__bits_48___UMT
> ? ? ? ? ? ?size <integer_cst 64>
>
> At least the change in size is problematic because the conversion
> outcome might differ after the replacement, in particular on
> big-endian targets.
>
> mainline does something slightly different with the same effects
> eventually (same testcase failure on sparc-solaris). The attached patch
> is a proposal to address this at the point where we already check
> for VCE in the access creation process, disqualifying scalarization
> for a VCE argument of non-integral size.
>
> We (AdaCore) have been using this internally for a while now.
> I also checked that it fixes the observable problem on sparc, then
> bootstrapped and regtested on i686-suse-linux.
>
> OK to commit ?
>
> Thanks in advance for your feedback,

Does this really cover all problematic cases?  Ah, the existing code
already rejects all VIEW_CONVERT_EXPRs down in the reference
chain.

I think much better would be to simply disallow any toplevel
VIEW_CONVERT_EXPR of BLKmode, so, something like

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c      (revision 186817)
+++ gcc/tree-sra.c      (working copy)
@@ -1001,9 +1001,10 @@ build_access_from_expr_1 (tree expr, gim

   /* We need to dive through V_C_Es in order to get the size of its parameter
      and not the result type.  Ada produces such statements.  We are also
-     capable of handling the topmost V_C_E but not any of those buried in other
-     handled components.  */
-  if (TREE_CODE (expr) == VIEW_CONVERT_EXPR)
+     capable of handling the topmost V_C_E if it has a suitable mode but
+     not any of those buried in other handled components.  */
+  if (TREE_CODE (expr) == VIEW_CONVERT_EXPR
+      && TYPE_MODE (TREE_TYPE (expr)) != BLKmode)
     expr = TREE_OPERAND (expr, 0);

   if (contains_view_convert_expr_p (expr))

Does that fix your problems, too?  If so I prefer that.

Thanks,
Richard.

> Olivier
>
> 2012-04-25 ?Olivier Hainque ?<hainque@adacore.com>
>
> ? ? ? ?* tree-sra.c (create_access): Additional IN_VCE argument, telling
> ? ? ? ?if EXPR is VIEW_CONVERT_EXPR input. ?Disqualify base if access size
> ? ? ? ?is not that of an integer mode in this case.
> ? ? ? ?(build_access_from_expr_1): Adjust caller.
>
> ? ? ? ?testsuite/
> ? ? ? ?* gnat.dg/sra_vce[_decls].adb: New testcase.
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]