[proposed fix for a big-endian bug in SRA affecting 4.5+ ] From: Olivier Hainque Subject: fix incorrect SRA transformation on non-integral VIEW_CONVERT argument Date: Wed, 25 Apr 2012 15:37:15 +0200 List-Archive: 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(D.1432); into: SR.12_17 = D.1435_3; T1b.F = VIEW_CONVERT_EXPR(SR.12_17); where we have and type 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, Olivier 2012-04-25 Olivier Hainque * 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. --- gcc-4.7.1/gcc/testsuite/gnat.dg/sra_vce.adb.~1~ 1970-01-01 01:00:00.000000000 +0100 +++ gcc-4.7.1/gcc/testsuite/gnat.dg/sra_vce.adb 2012-09-09 12:45:55.000000000 +0200 @@ -0,0 +1,12 @@ +-- { dg-do run } +-- { dg-options "-O1" } + +with SRA_VCE_decls; use SRA_VCE_decls; + +procedure SRA_VCE is +begin + PA(1).Z := 44; + if PA(1).Y /= Y0 then + raise program_error; + end if; +end; --- gcc-4.7.1/gcc/testsuite/gnat.dg/sra_vce_decls.ads.~1~ 1970-01-01 01:00:00.000000000 +0100 +++ gcc-4.7.1/gcc/testsuite/gnat.dg/sra_vce_decls.ads 2012-09-09 12:45:55.000000000 +0200 @@ -0,0 +1,23 @@ +package SRA_VCE_decls is + type u16 is range - 2 ** 15 .. 2 ** 15 - 1; + for u16'SIZE use 16; + + type Point is record + X, Y, Z : u16; + end record; + + for Point use record + X at 0 range 0 .. 15; + Y at 2 range 0 .. 15; + Z at 4 range 0 .. 15; + end record; + for Point'SIZE use 48; + + type Parray is array (Integer range 1 .. 1) of Point; + pragma pack (Parray); + + X0 : constant := 11; + Y0 : constant := 22; + Z0 : constant := 33; + PA : Parray := (1=> (X => X0, Y => Y0, Z => Z0)); +end; --- gcc-4.7.1/gcc/tree-sra.c.~1~ 2012-05-28 15:58:18.000000000 +0200 +++ gcc-4.7.1/gcc/tree-sra.c 2012-09-09 12:45:55.000000000 +0200 @@ -807,11 +807,11 @@ create_access_1 (tree base, HOST_WIDE_IN return access; } -/* Create and insert access for EXPR. Return created access, or NULL if it is - not possible. */ +/* Create and insert access for EXPR, argument of a VIEW_CONVERT_EXPR if + IN_VCE. Return created access, or NULL if it is not possible. */ static struct access * -create_access (tree expr, gimple stmt, bool write) +create_access (tree expr, gimple stmt, bool write, bool in_vce) { struct access *access; HOST_WIDE_INT offset, size, max_size; @@ -866,6 +866,18 @@ create_access (tree expr, gimple stmt, b } } + /* We must not let scalarization change the size of a VIEW_CONVERT_EXPR + argument, as the result would differ from the original conversion outcome + on big-endian targets. We enforce this by disqualifying the base of an + access when the size doesn't correspond to that of some integer mode, + assuming this would be the type-size of the replacement if it were to + take place. */ + if (in_vce && !SCALAR_INT_MODE_P (mode_for_size (size, MODE_INT, 0))) + { + disqualify_candidate (base, "Access of non-integral size in V_C_E."); + return NULL; + } + access = create_access_1 (base, offset, size); access->expr = expr; access->type = TREE_TYPE (expr); @@ -987,7 +999,7 @@ static struct access * build_access_from_expr_1 (tree expr, gimple stmt, bool write) { struct access *ret = NULL; - bool partial_ref; + bool partial_ref, in_vce; if (TREE_CODE (expr) == BIT_FIELD_REF || TREE_CODE (expr) == IMAGPART_EXPR @@ -1004,7 +1016,12 @@ build_access_from_expr_1 (tree expr, gim 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) - expr = TREE_OPERAND (expr, 0); + { + expr = TREE_OPERAND (expr, 0); + in_vce = true; + } + else + in_vce = false; if (contains_view_convert_expr_p (expr)) { @@ -1026,7 +1043,7 @@ build_access_from_expr_1 (tree expr, gim case COMPONENT_REF: case ARRAY_REF: case ARRAY_RANGE_REF: - ret = create_access (expr, stmt, write); + ret = create_access (expr, stmt, write, in_vce); break; default: