| Message ID | 56949BBA.60006@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 11, 2016 at 10:22 PM, kugan <kugan.vivekanandarajah@linaro.org> wrote: > When promote_function_mode and promote_ssa_mode changes the sign > differently, following is the cause for the problem in PR67714. > This is similar to PR65932 where sign change in PROMOTE_MODE causes problem > for parameter. But need a different fix there. One of the proposed fixes for PR65932 was to make PROMOTE_MODE work the same as promote_function_mode. That should fix both bugs, and avoid some of the weirdness necessary to work around the problem where they disagree. However, that fix is stalled, because it causes potential performance regressions for some older ARM versions. I've been meaning to look at that again. It is probably a better fix than the one you are proposing here if we can make it work. Jim
On 13/01/16 10:19, Jim Wilson wrote: > On Mon, Jan 11, 2016 at 10:22 PM, kugan > <kugan.vivekanandarajah@linaro.org> wrote: >> When promote_function_mode and promote_ssa_mode changes the sign >> differently, following is the cause for the problem in PR67714. > >> This is similar to PR65932 where sign change in PROMOTE_MODE causes problem >> for parameter. But need a different fix there. > > One of the proposed fixes for PR65932 was to make PROMOTE_MODE work > the same as promote_function_mode. That should fix both bugs, and > avoid some of the weirdness necessary to work around the problem where > they disagree. However, that fix is stalled, because it causes > potential performance regressions for some older ARM versions. I've > been meaning to look at that again. It is probably a better fix than > the one you are proposing here if we can make it work. Yes, making PROMOTE_MODE to work the same way as in promote_function_mode in arm will fix this. Can you please point me to the test cases that are regressing so that I can also start looking at them. Thanks, Kugan
On Tue, Jan 12, 2016 at 5:10 PM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: > Yes, making PROMOTE_MODE to work the same way as in > promote_function_mode in arm will fix this. Can you please point me to > the test cases that are regressing so that I can also start looking at them. The info is in here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65932 See the comments on gcc.target/arm/wmul-[123].c which no longer generate smulbb etc instructions, which are 16x16=32 expanding multiplies which are faster on some older parts that have them. They are present in armv5e and higher architecture versions. Kyrylo looked at this in November, but the situation looks even worse now, as some of the redundant sign extends are gone even before the first rtl pass. That may make it harder to get the smulbb instructions back. Jim
On Tue, Jan 12, 2016 at 5:40 PM, Jim Wilson <jim.wilson@linaro.org> wrote: > The info is in here > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65932 > See the comments on gcc.target/arm/wmul-[123].c which no longer > generate smulbb etc instructions, which are 16x16=32 expanding > multiplies which are faster on some older parts that have them. They > are present in armv5e and higher architecture versions. I forgot about the ldrub/ldrsb problem. ldrub is preferred, particularly for older targets, e.g. thumb1, as it accepts more addressing modes than ldrsb. We can't get ldrub if PROMOTE_MODE doesn't do unsigned extension. So we have a number of bad choices here 1) We can remove sign-changing promotions from PROMOTE_MODE, and accept slower code for pre-thumb2 architectures. 2) We can add sign-changing promotions to function_promote_mode, and accept a minor ABI change. 3) We can add strange and probably fragile extensions to the middle end to work around the ARM back end problem. 4) We can just leave the ARM port broken and let it occasionally generate incorrect code. Option 4 is the one that we've been using for the last 8 months or so. I think we should do either 1 or 2, though that depends on what the ARM maintainers are willing to accept. Jim
diff --git a/gcc/expr.c b/gcc/expr.c index bd43dc4..6a2b3c0 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -9710,7 +9710,25 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode, gimple_call_fntype (g), 2); else - pmode = promote_ssa_mode (ssa_name, &unsignedp); + { + tree rhs; + gimple *stmt; + /* When this is a SSA copy from a value returned from a + function call, use the corect promoted sign for SUBREG_PROMOTED_P + (PR67714). */ + if (code == SSA_NAME + && is_gimple_assign (g) + && (rhs = gimple_assign_rhs1 (g)) + && TREE_CODE (rhs) == SSA_NAME + && (stmt = SSA_NAME_DEF_STMT (rhs)) + && gimple_code (stmt) == GIMPLE_CALL + && !gimple_call_internal_p (stmt)) + pmode = promote_function_mode (type, mode, &unsignedp, + gimple_call_fntype (stmt), + 2); + else + pmode = promote_ssa_mode (ssa_name, &unsignedp); + } gcc_assert (GET_MODE (decl_rtl) == pmode); temp = gen_lowpart_SUBREG (mode, decl_rtl); diff --git a/gcc/testsuite/gcc.target/arm/pr67714.c b/gcc/testsuite/gcc.target/arm/pr67714.c index e69de29..355b559 100644 --- a/gcc/testsuite/gcc.target/arm/pr67714.c +++ b/gcc/testsuite/gcc.target/arm/pr67714.c @@ -0,0 +1,26 @@ + +/* PR target/67714 */ +/* { dg-do-run } */ +/* { dg-options "-O1" } */ + +unsigned int b; +int c; + +signed char fn1 () +{ + signed char d; + for (int i = 0; i < 1; i++) + d = -15; + return d; +} + +int main() +{ + for (c = 0; c < 1; c++) + b = 0; + char e = fn1(); + signed char f = e ^ b; + __builtin_printf("checksum = %x\n", (int)f); + if ((int)f != 0xfffffff1) + __builtin_abort (); +}